Add authn-oidc configurability solution design

[gl-johnson]

Quick solution design addressing some requested improvements in configurability for the OIDC authenticator:

• Support for configurable ca-cert variable in authn-oidc config
• Support for HTTPS_PROXY environment variable
• Improved debug logging

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

• The CHANGELOG has been updated, or
• This PR does not include user-facing changes and doesn't require a
  CHANGELOG update

Test coverage

• This PR includes new unit and integration tests to go with the code
  changes, or
• The changes in this PR do not require tests

Documentation

• Docs (e.g. READMEs) were updated in this PR
• A follow-up issue to update official docs has been filed here: [insert issue ID]
• This PR does not require updating any documentation

Behavior

• This PR changes product behavior and has been reviewed by a PO, or
• These changes are part of a larger initiative that will be reviewed later, or
• No behavior was changed with this PR

Security

• Security architect has reviewed the changes in this PR,
• These changes are part of a larger initiative with a separate security review, or
• There are no security aspects to these changes

[codeclimate]

Trailing spaces

[codeclimate]

Trailing spaces

[codeclimate]

Trailing spaces

[codeclimate]

Trailing spaces

[codeclimate]

Trailing spaces

[john-odonnell]

Given that this is the AuthnOIDC V2 process:

Client (UI/CLI)             Conjur                    OIDC Provider
|-------- providers ------->|                         |
|                           |- openid-configuration ->|
|                           |<------- response -------|
|<------- response ---------|                         |
|                           |                         |
|-------- authz ------------------------------------->|
|<------- authz token --------------------------------|
|                           |                         |
|------- authz token ------>|                         |
|                           |------ authz token ----->|
|                           |<----- id token ---------|
|<----- access token -------|                         |
|                           |                         |

and that we've experienced these cert issues:

1. during provider discovery, and
2. either:
   • when the provider is using custom certificates, or
   • when a proxy sits between Conjur and the provider

are there cases where the UI and CLI will need a similar solution to properly send the authz token request? If so, and if we're storing the custom cert in Conjur, how can we expose it the UI and CLI?

[gl-johnson]

Good question. My initial impression would be that we do not need to add anything to the UI/CLI. In those cases the request originates in the browser rather than from the UI/CLI server and we can assume that a user of the OIDC provider will have the necessary certs installed on their machine. Will do some validation to be sure though.

[jvanderhoof]

As we're using environment variables, will there be any issues in regards to thread safety? If two different OIDC authenticators are configured and each uses different CAs, if authentication requests come in simultaneously, will the second SSL_CERT_FILE set be used in the first request?

[gl-johnson]

I had concerns about this as well, but based on discussion with @ismarc I was under the impression the change to the environment should be localized to the thread which is handling that particular authn-oidc request. However I'm not the most well-versed in how Rails handles concurrency so I'll look into it a bit further.

Worst case I imagine would be adding a mutex to make sure only one request is accessing the environment variable until cleanup has occurred (if acceptable from a performance standpoint)

[andytinkham]

Addressing this will be critical for security sign-off on the design and implementation.

[jvanderhoof]

@gl-johnson, after some digging through the OpenID Connect gem, I think there's a simple option to enable CAs. OpenID Connect includes the ability to pass configuration to the underlying Faraday library: https://github.com/nov/openid_connect/blob/2fdafc3802aca1967790b079cb4e58ce5c4e9c93/spec/openid_connect_spec.rb#L45

(note: docs in are terrible, but the code is well tested. I've found the tests more valuable for "how-to" guidance.)

It looks like this exposes the Faraday SSL config options.

First, a method to create temp files that exist for the block duration:

# Create a temp file which exists for the duration of the block
def temp_ca_certificate(certificate_content, &block)
  ca_certificate = Tempfile.new('ca_certificates')
  begin
    ca_certificate.write(certificate_content)
    ca_certificate.close
    block.call(ca_certificate)
  ensure
    ca_certificate.unlink   # deletes the temp file
  end
end

Then add a certificate to an X509 cert store (so we can handle CA chains):

store = OpenSSL::X509::Store.new

if @authenticator.ca_cert.present?
  temp_ca_certificate(@authenticator.ca_cert) do |file|
    store.add_file(file.path)
  end
else
  # Auto-include system CAs unless a CA has been defined
  store.set_default_paths
end

OpenIDConnect.http_config do |config|
  config.ssl.cert_store = store
end

We should be able to add this where we configure an OpenID Connect Client.

As OpenIDConnect.http_config looks to be a singleton, this option isn't thread safe, but feels cleaner than modifying environment variables (which is likely to impact all calls which use Net::HTTP under the hood).

[jvanderhoof]

As Faraday has a proxy configuration, it's also possible the Faraday configuration approach could be used to support the Faraday proxy configuration: https://lostisland.github.io/faraday/#/customization/proxy-options

[gl-johnson]

Unfortunately the http_config can only be configured once. After it has been called it will not overwrite the existing config. See this open issue: nov/openid_connect#84

The same goes for the SWD and Webfinger gems as well, so we'd end up having to modify all 3 gems to make it reconfigurable. I initially went down the path you're describing and it does work, but only after modifying the http_config behavior of the libraries

[codeclimate]

Trailing spaces

[codeclimate]

Trailing spaces

[codeclimate]

Trailing spaces

[codeclimate]

Trailing spaces

[codeclimate]

Trailing spaces

[codeclimate]

Code Climate has analyzed commit 07be592 and detected 235 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	235

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 87.1% (-1.2% change).

View more on Code Climate.

[jvanderhoof]

We can sort it out later, but this approach won't work for CA certs that are chains (the certificate creation will fail). For those, we need to write the cert to a temp file and import it (example I previously posted is part of the authn-jwt rewrite, which includes support for CA chains).

[gl-johnson]

There's already a parse_certs method in the CertUtils module which I imagine can be repurposed to do exactly what you're describing. But yes we would need slightly modified logic to create separate temp files/symlinks for each cert that gets parsed from the chain

[jvanderhoof]

Really just making sure our eventual tests include examples with the configured CA certificate being a single certificate as well as a certificate chain.

[niteshtaneja]

Is this the only visible user facing change needed to support ca-certs?

[niteshtaneja]

For customers upgrading from v13.0, do they just need to add this variable in the auth policy for custom certs to work?

[gl-johnson]

Yes, the ca-cert variable will be optional and is the only UX change.

[niteshtaneja]

The ideal response to all these questions would be a "Yes" I am assuming

[niteshtaneja]

Do we have a sense of how much additional work it will be to add the validation? If its not a valid cert, whats the worst thats going to happen?

[gl-johnson]

In all likelihood this will be a non-issue now that I've thought about it more. We will use the OpenSSL Ruby library to parse the cert(s) from the ca-cert value, and it will produce an error we can easily handle if it is malformed in any way.

The worst that will happen is the TLS connection would fail and they would have to fix the variable value.

[jvanderhoof]

I'd recommend we add the error response and log message for a misconfigured CA (we'll need to handle malformed x509 certs and incorrect CA) to this document for completeness.

[andytinkham]

We also need to explore what might happen with malicious certs, not just malformed.

[adamouamani]

@gl-johnson @jvanderhoof @niteshtaneja

love the collaboration in this PR 👍

I defer to you, Jason, Andy and Nitesh - but, I'm wondering if we are solving this the right way. Why add custom ca certificate to ca-cert variable and build a wrapper to add to /etc/ssl/certs and all the logic around that when the workaround seems better. Additionally, it's less error prone and you only do it once instead of for every policy/authn_oidc authenticator? Is this to better support the container lifecycle and k8s deployments? Also, why tie this to a specific authenticator - seems like something better at system level (like trusted proxies)

Also, are we trimming trailing spaces and CRLF?

[niteshtaneja]

Is that what we are doing?

[gl-johnson]

I mentioned this to @jtuttle and it sounds like it is preferable to not introduce a new configuration, so yes it would be based on whether CONJUR_LOG_LEVEL is set to debug.

[jvanderhoof]

Agreed! Our docs provide guidance to toggle Conjur into and out of debug mode while troubleshooting an authenticator.

[andytinkham]

Are these log messages we control? Either way, we need to be certain we don't inadvertently cause disclosures in debug mode.

[adamouamani]

@gl-johnson Here is a current issue in Jira that OIDC does not give back stack trace for error logging / troubleshooting
https://ca-il-jira.il.cyber-ark.com:8443/browse/CNJR-2105

[gl-johnson]

These would be log messages outside our control.

OpenIdConnect and SWD logs will contain debug messaging mainly about the HTTP requests to the OIDC provider. The OpenSSL logs may also be useful for lower level issues. When working on a customer case where they had TLS issues in authn-oidc, the OpenSSL logs helped us discover that the CA file they added manually to Conjur did not have the correct permissions.

At most I would expect these logs to include the filepath where OpenSSL expects the CA cert to be, and possibly a public key.

[jvanderhoof]

I like the approaches summarized in this document.

I'm adding this comment here as it's related to the CA configuration, but this should NOT be part of this effort.

Longer term, we should seriously consider replacing the OpenIDConnect gem with an internal implementation. We make only two calls:

• Discovery: a simple URL get request for which we need to parse the response into JSON
• Exchange code for token:

  POST /token HTTP/1.1
  Host: server.example.com
  Content-Type: application/x-www-form-urlencoded
  Authorization: Basic czZCaGRSa3F0MzpnWDFmQmF0M2JW
  
  grant_type=authorization_code&code=SplxlOBeZQQYbYS6WxSbIA
  &redirect_uri=https%3A%2F%2Fclient.example.org%2Fcb 
  

Once the Authn-JWT refactor is completed, we can use Authn-JWT strategy to validate the resulting OIDC JWT token.

The key driver for the re-write is to provide a generic HTTP workflow based on Faraday for all authenticator network requests. This will allow us to build a unified approach to authenticator network calls.

[andytinkham]

I see no evidence of security being considered in this design and no security considerations section exists. There are several pieces of the individual comments that need to be addressed before this design can be approved (see my other comments). Because this is a change to authenticator behavior and touching on certificate handling, we need to do a full security review and threat modelling session as well.

[andytinkham]

How do we make sure that this wrapper method only injects the certificates we intend and isn't available for use by an attacker?

[gl-johnson]

It will only going to inject the cert(s) provided via the authenticator policy, so unless an attacker has the ability to change variable values in the authenticator policy I would think it's a non-issue.

I can't reply to your other comments for some reason but I was curious about what a malicious cert would look like and if that is something we can realistically detect and handle.

Also just pushed an updated doc with a few security considerations I could think of.

[andytinkham]

You go from discussing the SWD gem which was only just referenced above to talking about modified code. Are you intending to modify the gem's code? More is needed about how this modified code could affect the other authns.

[gl-johnson]

No gem code would be modified. This is in reference to this generic OAuth code which is used by the status webservice of multiple authenticators to verify connectivity to the configured provider. It would need to include the custom cert in the case of authn-oidc without impacting the other authenticators that also use it.

Will push an update soon that is hopefully more clear. When I started this doc we were planning on modifying the gem code but hopefully that is not necessary with this approach.

[andytinkham]

We also need to explore what might happen with malicious certs, not just malformed.

[andytinkham]

Are these log messages we control? Either way, we need to be certain we don't inadvertently cause disclosures in debug mode.

[andytinkham]

Addressing this will be critical for security sign-off on the design and implementation.

[adamouamani]

Are we supporting both http_proxy and https_proxy?

Also, I believe these variables are typically lower case, can we check for uppercase or lowercase?

[gl-johnson]

That's a function of the underlying HTTP library. After making the gem update it will be Faraday with a Net::HTTP adapter. In my testing it honored both environment variables.

[adamouamani]

Current issue with authn_oidc debugging errors and stack traces - https://ca-il-jira.il.cyber-ark.com:8443/browse/CNJR-2105

[adamouamani]

Is this a single CA cert or Chain? How would you handle multiple certs - would these need to be concatenated together?

[gl-johnson]

It should support both, which we can do via a pre-existing parse_certs method. In the policy they would be concatenated together, then Conjur would separate them into individual certs for the purpose of adding them to the truststore.

[adamouamani]

Are these environment variables and is this something we want to add to docs or kb?
Would this help resolve this issue - https://ca-il-jira.il.cyber-ark.com:8443/browse/CNJR-2105

[gl-johnson]

This is the implementation that would likely go in an initializer in Conjur.

For a user this would be enabled by setting CONJUR_LOG_LEVEL to debug per the docs https://docs.cyberark.com/AAM-DAP/13.0/en/Content/Operations/Services/logs.htm#Enabledebuglogmessages

[adamouamani]

@gl-johnson would it be better to test against Okta or Identity as an E2E test with Custom CA Cert and Proxy? Also, How are we setting up Proxy - can we test with upper and lower case https_proxy.

[gl-johnson]

To do this we would need an Identity or Okta instance using a private CA which seems like a large ask from infra. Our Keycloak server uses already uses a self-signed cert which is functionally the same as a private CA anyway.

I don't know that we necessarily need an automated test for the environment variables in this approach, since we'd be testing whether a standard Ruby library (Net::HTTP) handles them correctly as opposed to anything within Conjur. Similarly for the configurable ca-cert change, OpenSSL doesn't care whether a trusted cert belongs to a CA or a proxy server so we'd be double-testing in a sense.

If we do need tests against a proxy server we would likely have to setup an nginx server with a self-signed cert as a forward proxy for Conjur

[adamouamani]

@gl-johnson do we currently have any e2e or integration tests for following:

• self-signed cert (keycloak or okra/identity)
• certificate chain (keycloak or okra/identity)
• https_proxy & HTTPS_PROXY
• concurrent writing of requests to cert store (are we making any backup of this file beforehand? Is there any risk of file corruption and are we leveraging built in linux tools - update-ca-certs/trust)

[gl-johnson]

Keycloak is already configured with a self-signed cert which will be easy to test against. For a cert chain we'd need to generate a CA/intermediate chain and reload Keycloak with an updated server config - although I think it is sufficient cover cert chains in unit tests. We can easily test that a cert chain is parsed correctly and that the cert store contains all certs in the chain.

Support for HTTPS_PROXY/https_proxy is not a standard, nor is it a function of Conjur - rather the underlying library that makes the outgoing connection. The previous library chose to use the same proxy (http_proxy) for both HTTP and HTTPS requests which I imagine is the case 99% of the time. I personally think it is overkill to have e2e tests requiring an additional service when we can easily confirm via some manual smoke testing.

Ruby's Tempfile library ensures uniqueness so concurrent read/write operations should not be possible as far as the cert itself is concerned. We will need to add some logic to ensure we handle collisions of the symlinks properly should they happen, but if anything they would link back to the same cert (possibly stored in multiple locations) and still allow the connection to succeed.

[adamouamani]

Thanks @gl-johnson for response

I'm fine with #1 (keycloak) so long as we do individual cert and chain which I believe we are already doing/planning!! 👍

For #2 https_proxy seems like it would be good candidate for a happy path e2e test - I understand this is a 3rd party library but would be good to have test that verifies this all works and if gem changes we will be notified of any breakage in pipeline vs manual testing from release to release. let me know what you think..

For # 3 seems like this is handled already - do we have any tests to confirm there is no issue?

For K8s Follower - need to understand if this is valid scenario and how a rootless user would update certstore if connecting to K8s-follower for authn-oidc v2

[gl-johnson]

If it was something under our control I would agree, but ultimately the proxy determination in this case is handled by a standard library in Ruby. This code is extremely unlikely to change., plus I imagine we already make this assumption with every other network call that Conjur makes to an external provider. It works like so:

1. Construct env var name using scheme of the request, should result in either http_proxy or https_proxy
2. If HTTP, favor http_proxy if it exists, otherwise use HTTP_PROXY
3. If HTTPS, favor https_proxy if it exists, otherwise use HTTPS_PROXY

The older version of OpenIDConnect used the not very well-maintained httpclient, which loaded the proxy settings itself and does not consider the scheme of the request when looking at environment variables.

As far as I can tell the K8s follower does not handle authn-oidc requests, but if you have information otherwise then I can look into it further.

[adamouamani]

How will this be impacted by pending FIPS/OpenSSL3 changes?

[gl-johnson]

I haven't found any indication that OpenSSL3 will have breaking changes for applications, but if so it should be covered by the tests

[adamouamani]

@gl-johnson What if customer is running Conjur with Rootless Podman? They will probably not have permissions to update /etc/ssl/certs

[adamouamani]

@gl-johnson What if customer is running Conjur with Rootless Podman? Will they have permissions to update /etc/ssl/certs

Actually for rootless podman we run as root in container so this is fine. In k8s-follower we do not run as root - will this be an issue?

[andytinkham]

My concerns are all addressed.