-
Notifications
You must be signed in to change notification settings - Fork 886
feat: add azure oidc PKI auth instead of client secret #9054
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #7266
What this does
This implements Azure's Certificate Credentials for OIDC providers. This uses a client certificate and key instead of
client_secret
for oauth2 auth.To use
There implementation is a super-set of the standard. So this might work for other providers, however I am sure they probably all have their own unique quirks. So I am not going to try and make this compatible with other providers until we get those requests.
Implementation
I just wrote a wrapper for the existing oauth2 config. So the oauth flow remains the same, I just add to the oauth2 context on the
Exchange
call:coder/coderd/oauthpki/oidcpki.go
Lines 124 to 127 in 74f9245
For the
TokenSource
, I had to write my own. I used the internal one in the oauth2 package as a guide:coder/coderd/oauthpki/oidcpki.go
Line 170 in 9c73346
Testing
The unit tests only focus on asserting the client requests to the IDP. I do not attempt to actually implement some fake IDP.
The first is
TestAzureADPKIOIDC
which tests theExchange
method adds the correctclient_assertion
for the PKI auth.The second is a more e2e test for the oauth2 flow,
TestSavedAzureADPKIOIDC
. It mocks out all the responses by making a fake http.Client with a transport. I did this because I wanted to use a realoauth2.Config
to actually invoke the right code paths. Theoauth2.Config
is what sets the auth payload in the header or the params. The existing fake oidc code does not use theoauth2
package, so it didn't allow me to assert the proper request payloads.I used actual response from a real Azure AD instance, and stubbed out some of the JWTs that do not do anything. This means I can test both the initial
Exchange
and the token refresh and assert the actualhttp.Requests
sent have the right authoriazation fields.Errors
If you provide a key + cert pair that is invalid: