Insufficient detail logged on connection failure to IdP metadata URL

[wfchandler]

This issue arose recently when a customer is attempting to register a new SAMP IdP provider.

Nexus was unable to connect to the metadata URL provided, but we do not log enough information to say why the connection failed, returning only "error sending request for url".

Error Response:
status: 400 Bad Request;
headers: {
  "content-type": "application/json",
  "x-request-id": "xxx",
  "content-length": "248",
  "date": "Fri, 12 Nov 2024 22:52:59 GMT"
};
value:Error {
  error_code: Some("InvalidValue"),
  message: "unsupported value for \"url\": error querying url: error sending request for url (https://sso.example- 
  keycloak.com/realms/myrealm/protocol/saml/descriptor)",
  request_id: "xxx"
}

We have a couple similar issues, #6717 and #6716 around vague connection errors. The underlying issue is probably related to seanmonstar/reqwest#2342, which has some suggestions around ways to extract more detail.

A systematic way to ensure we're logging sufficient detail on chained errors seems necessary.

[davepacheco]

For what it's worth, we do have good infrastructure here using https://github.com/oxidecomputer/slog-error-chain (which, despite the name, is useful outside of logging, too). This is relatively recent and we may need to go through the places mentioned here and make sure they're using it to provide a complete message to the user.

[wfchandler]

@davepacheco thanks, that's good context. I see we're using slog-error-chain pretty extensively in our internal errors, e.g. gateway::error::SpLookupError, which seems like a nice solution.

However, on omicron_common::api::external::Error it looks like we're deliberately using strings in the error type to avoid leaking internal information, does that seem right? This requires each call site to create an error chain, so unless the source error is already using InlineErrorChain in its Display impl we're probably not going to get one.

Would a constructor along the lines of the example below be a security risk?

pub fn internal_error_from_err(err: &dyn std::error::Error) -> Error {
    Error::InternalError {
        internal_message: format!("{}", InlineErrorChain::new(&err)),
    }
}

[david-crespo]

Was going to make a separate issue for this but: it would also be smart to improve the error text in the API response to make clearer what's going wrong. Like "idp_metadata_source URL failed to respond".