Add refresh_token logic and tests to OIDC API

[gl-johnson]

Desired Outcome

Generalize Conjur's existing OIDC API to accept refresh tokens in the request body on POST requests to /authn-oidc(/:service_id)/:account/authenticate. This endpoint should accept form-encoded data representing an OIDC credential in the request body, and respond with a Conjur access token.

Implemented Changes

• Update the authenticate_controller to parse the request body to support the following types of OIDC request
  • Authorization code (authenticate against OIDC provider)
  • Refresh token (re-authenticate against OIDC provider)
  • ID token (authenticates to Conjur)
• Update authenticate_handler and strategy to route requests accordingly
• Add relevant rspec tests/cucumber scenarios

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

[john-odonnell]

Looks good! Left some feedback points.

[john-odonnell]

This step still uses a get request - if we force it to use post, it might be a better test of our generalization. (Maybe this is a separate test case from the one highlighted here)

[gl-johnson]

Isn't this step emulating the redirect from the OIDC provider back to Conjur containing the code as a query param? My understanding is that this would always be a GET request but maybe I'm mistaken

[john-odonnell]

I don't think so. They're similar, but:

1. The OIDC redirect doesn't contain all the information it needs - it needs to be supplemented with both nonce and code_verifier parameters.
2. Conjur is responding to the code with an access token - the requester should be the Conjur client.

In terms of the UI:

1. The UI server redirects to the OIDC provider
2. The OIDC provider redirects back to the UI server using a GET request with code and state query parameters
3. The UI server will evaluate the state, and send the code, nonce, and code_verifier values to Conjur in a POST request
4. Conjur exchanges the code with the OIDC provider, and responds to the UI server with an access token

[john-odonnell]

Given that the rescue-else blocks only apply to the login in this else, would it make sense to embed them? Especially given the log_audit_failure references authenticator_input.

else
  begin
    input = ...
  rescue => e
    ...
  else
    authenticate(input)
  end
end

[john-odonnell]

Although, we probably need to create audit logs in both cases. Maybe it's structured best as-is, and we just need to make sure we're generating accurate audit logs.

[gl-johnson]

The authenticator_input obj seems to sufficiently cover the incoming request in both cases imo. The request body is logged in the credentials field which would reflect the refresh_token, authorization code, nonce, code_verifier, etc. that was part of the failing request.

[john-odonnell]

I wonder if this has potential security implications. Given that the client already has their valid refresh token, maybe we should avoid re-transmitting it on each authentication request.

[john-odonnell]

This should probably be an error case. Given this discussion, we might want to require a nonce input, and choose to ignore it if that's what validation requires.

[john-odonnell]

I think part of the reason why the AuthenticationHandler class was created was to see if it could be adopted as the new standard for all our authn schemes (see commit 1c458bb in this draft PR from @jvanderhoof).

Given that this Strategy class could become a catch-all for what could be described as 3 distinct strategies, maybe we should create a class for each:

module Strategies
  class Code
    ...
  end

  class RefreshToken
    ...
  end

  class IdToken
    ...
  end

  class Common
    ...
  end
end

[john-odonnell]

Ack - realizing now that this is a separate task. Ignore for now!

[john-odonnell]

Thanks for the updates - LGTM 👍