conjur icon indicating copy to clipboard operation
conjur copied to clipboard

Add authn-oidc configurability solution design

Open gl-johnson opened this issue 11 months ago • 9 comments

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
  • [x] 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
  • [x] 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]
  • [x] 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
  • [x] 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
  • [x] There are no security aspects to these changes

gl-johnson avatar Jul 27 '23 17:07 gl-johnson

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?

john-odonnell avatar Aug 02 '23 19:08 john-odonnell

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.

gl-johnson avatar Aug 02 '23 23:08 gl-johnson

@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 avatar Aug 03 '23 17:08 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

jvanderhoof avatar Aug 03 '23 17:08 jvanderhoof

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: https://github.com/nov/openid_connect/issues/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

gl-johnson avatar Aug 03 '23 17:08 gl-johnson

Code Climate has analyzed commit 07be592d 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.

codeclimate[bot] avatar Aug 03 '23 20:08 codeclimate[bot]

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.

jvanderhoof avatar Aug 10 '23 16:08 jvanderhoof

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

adamouamani avatar Aug 11 '23 03:08 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?

adamouamani avatar Aug 16 '23 17:08 adamouamani