Add security considerations and some feedback updates

design/authenticators/authn_oidc/oidc_configuration_enhancements.md

@@ -116,6 +116,12 @@ And its usage:
 end
 ```
 
+We can easily support cert chains and perform validation using the existing [parse_certs](https://github.com/cyberark/conjur/blob/48e95904a5ee8cda1503db9f5744ff6eefcecbdb/app/domain/conjur/cert_utils.rb#L11C10-L11C10) method. This will produce an error message of the form:
+```
+Invalid certificate:
+#{cert} (#{error.message})
+```
+
 As for the `HTTPS_PROXY` issue, it appears to be fixed in a more recent version of OpenIDConnect gem which 
 migrated from the `httpclient` library to `faraday` for handling outbound connections. Therefore we should 
 simply be able to bump this gem version (assuming no new issues arise) in order to address this issue.
@@ -125,7 +131,6 @@ via a code snippet like below:
 ```
 OpenIDConnect.logger = WebFinger.logger = SWD.logger = Rack::OAuth2.logger = Rails.logger
 OpenIDConnect.debug!
-# Include cert-related debug messages?
 OpenSSL.debug = true
 ```
 Running the above in an initializer and will enable debug logging on the OpenIDConnect gem and all its 
@@ -148,10 +153,22 @@ endpoint:
   - validate authenticator status and provider connectivity
   - fetch provider keys for decoding tokens
 
-This should not interfere with the proposed solution, but it is important to note since the modified code will be 
+This should not interfere with the proposed solution, but it is important to note since the [modified discovery code](https://github.com/cyberark/conjur/blob/48e95904a5ee8cda1503db9f5744ff6eefcecbdb/app/domain/authentication/o_auth/discover_identity_provider.rb#L31C40-L31C40) will be 
 used across different authenticators to invoke the provider discovery endpoint `/.well-known/openid-configuration`. 
 It will be necessary that the other authenticators are unaffected by this change.
 
+### Security
+The solution proposes adding a CA certificate (or chain) to the OpenSSL truststore of the Conjur container for the duration of certain HTTP requests. This involves writing certificate contents to the container volume, creating a symlink in the OpenSSL truststore, and yielding for the duration of the specific logic. Once this logic is complete, the CA certificate and symlink will be removed.
+
+#### Security Risks and Mitigations
+**Certificate Management**: Temporary changes to truststores can lead to mismanagement of certificates, resulting in leftover or unused certificates remaining in the truststore. Also we need to be sure that there are no circumstances in which the certificate management process is interrupted.
+
+**Mitigation**: Implement a thorough certificate management test suite that validates the addition, usage, and removal of certificates.
+
+**Log Verbosity and Information Disclosure**: Debugging TLS issues with an OIDC provider is overly difficult with the current debug logging. We would like to expose additional logging from the OpenIDConnect gem (which makes requests to the provider) and OpenSSL (which performs TLS verification based on the configured truststore). This requires that we do not disclose any sensitive information with the additional debug logs.
+
+**Mitigation**: Verify that logs for each of the components do not disclose sensitive data, including certificate keys, metadata, or filepaths.
+
 ### Testing
 The testing will depend on the final implementation, but at the very least the wrapper method will need to be 
 well-covered by unit tests.
@@ -160,13 +177,15 @@ well-covered by unit tests.
 1. OpenSSL truststore contains the CA cert symlink while yielding
 1. OpenSSL truststore symlink is removed after yielding
 1. Hash collision in truststore does not produce an error but instead increments the file extension of the symlink
-1. Temp file/dir exists while yielding
-1. Temp file/dir contains expected certificate content while yielding
-1. Temp file/dir is removed after yielding
+1. Cert chains work in addition to individual CA certs
+1. Malformed certificates produce the expected error message
+1. Temp file/dir/symlink exists while yielding
+1. Temp file/dir/symlink contains expected certificate content while yielding
+1. Temp file/dir/symlink is removed after yielding
 
 #### Unit tests (authenticator data object)
 1. Default `ca-cert` value is nil
-1. Set value matches expected
+1. Set value matches expected value
 
 #### Cucumber tests
 One or more of our Keycloak OIDC scenarios should be updated to use a `ca-cert` value provided via Conjur 
@@ -175,8 +194,11 @@ where the variable is not set can be covered by the Okta examples.
 
 ### Open Questions
 - Do we need validation that `ca-cert` is a valid cert before attempting to use it?
+Answer: Yes - this will be done while parsing the cert (or cert chain) into an OpenSSL::X509::Certificate object.
 - Should debug logging of OIDC be enabled by default when Conjur is running in debug?
+Answer: Yes, it should use the same debug toggle as the rest of Conjur.
 - Should OpenSSL debug logs be included?
+Answer: Yes, since some cert issues may occur outside the scope of the HTTP request, such as when a cert file has incorrect permissions.
 
 ## Tasks
 ### 1. Update the OpenIDConnect gem (1 pt)