From 8ba23b1ebcc5bd64636a17fc047bbf06deba5ccd Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Aug 2023 10:38:03 -0500 Subject: [PATCH 01/14] feat: add azure oidc PKI auth instead of client secret --- cli/server.go | 25 +++++---- coderd/azureauth/oidcpki.go | 94 ++++++++++++++++++++++++++++++++++ codersdk/deployment.go | 11 ++++ site/src/api/typesGenerated.ts | 5 ++ 4 files changed, 126 insertions(+), 9 deletions(-) create mode 100644 coderd/azureauth/oidcpki.go diff --git a/cli/server.go b/cli/server.go index 55a4db844723f..ad1742316b0ab 100644 --- a/cli/server.go +++ b/cli/server.go @@ -33,6 +33,8 @@ import ( "sync/atomic" "time" + "github.com/coder/coder/coderd/azureauth" + "github.com/coreos/go-oidc/v3/oidc" "github.com/coreos/go-systemd/daemon" embeddedpostgres "github.com/fergusstrange/embedded-postgres" @@ -551,7 +553,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. } } - if cfg.OIDC.ClientSecret != "" { + if true || cfg.OIDC.ClientSecret != "" { if cfg.OIDC.ClientID == "" { return xerrors.Errorf("OIDC client ID be set!") } @@ -578,15 +580,20 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. if slice.Contains(cfg.OIDC.Scopes, "groups") && cfg.OIDC.GroupField == "" { cfg.OIDC.GroupField = "groups" } + oauthCfg := &oauth2.Config{ + ClientID: cfg.OIDC.ClientID.String(), + ClientSecret: cfg.OIDC.ClientSecret.String(), + RedirectURL: redirectURL.String(), + Endpoint: oidcProvider.Endpoint(), + Scopes: cfg.OIDC.Scopes, + } + jwtCfg, err := azureauth.NewJWTAssertion(oauthCfg) + if err != nil { + return xerrors.Errorf("configure azure auth: %w", err) + } options.OIDCConfig = &coderd.OIDCConfig{ - OAuth2Config: &oauth2.Config{ - ClientID: cfg.OIDC.ClientID.String(), - ClientSecret: cfg.OIDC.ClientSecret.String(), - RedirectURL: redirectURL.String(), - Endpoint: oidcProvider.Endpoint(), - Scopes: cfg.OIDC.Scopes, - }, - Provider: oidcProvider, + OAuth2Config: jwtCfg, + Provider: oidcProvider, Verifier: oidcProvider.Verifier(&oidc.Config{ ClientID: cfg.OIDC.ClientID.String(), }), diff --git a/coderd/azureauth/oidcpki.go b/coderd/azureauth/oidcpki.go new file mode 100644 index 0000000000000..6db192bcc526c --- /dev/null +++ b/coderd/azureauth/oidcpki.go @@ -0,0 +1,94 @@ +package azureauth + +import ( + "context" + "crypto/rsa" + "crypto/sha1" + "crypto/x509" + "encoding/base64" + "encoding/pem" + "time" + + "github.com/google/uuid" + + "github.com/golang-jwt/jwt/v4" + + "golang.org/x/xerrors" + + "golang.org/x/oauth2" +) + +// JWTAssertion is used by Azure AD when doing OIDC with a PKI cert instead of +// a client secret. +// https://learn.microsoft.com/en-us/azure/active-directory/develop/certificate-credentials +type JWTAssertion struct { + *oauth2.Config + + // ClientSecret is the private key of the PKI cert. + // Azure AD only supports RS256 signing algorithm. + clientKey *rsa.PrivateKey + // Base64url-encoded SHA-1 thumbprint of the X.509 certificate's DER encoding. + x5t string +} + +func NewJWTAssertion(config *oauth2.Config, pemEncodedKey string, pemEncodedCert string) (*JWTAssertion, error) { + rsaKey, err := DecodeKeyCertificate([]byte(pemEncodedKey)) + if err != nil { + return nil, err + } + + block, _ := pem.Decode([]byte(pemEncodedCert)) + hashed := sha1.Sum(block.Bytes) + + return &JWTAssertion{ + Config: config, + clientKey: rsaKey, + x5t: base64.StdEncoding.EncodeToString(hashed[:]), + }, nil +} + +// DecodeKeyCertificate decodes a PEM encoded PKI cert. +func DecodeKeyCertificate(pemEncoded []byte) (*rsa.PrivateKey, error) { + block, _ := pem.Decode(pemEncoded) + key, err := x509.ParsePKCS1PrivateKey(block.Bytes) + if err != nil { + return nil, xerrors.Errorf("failed to parse private key: %w", err) + } + + return key, nil +} + +func (ja *JWTAssertion) AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string { + return ja.Config.AuthCodeURL(state, opts...) +} + +func (ja *JWTAssertion) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { + now := time.Now() + token := jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "aud": ja.Config.Endpoint.TokenURL, + "exp": now.Add(time.Minute * 5).Unix(), + "jti": uuid.New().String(), + "nbf": now.Unix(), + "iat": now.Unix(), + + // TODO: Should be app GUID, not client ID. + "iss": ja.Config.ClientID, + "sub": ja.Config.ClientID, + }) + token.Header["x5t"] = ja.x5t + + signed, err := token.SignedString(ja.clientKey) + if err != nil { + return nil, xerrors.Errorf("failed to sign jwt assertion: %w", err) + } + + opts = append(opts, + oauth2.SetAuthURLParam("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"), + oauth2.SetAuthURLParam("client_assertion", signed), + ) + return ja.Config.Exchange(ctx, code, opts...) +} + +func (ja *JWTAssertion) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.TokenSource { + return ja.Config.TokenSource(ctx, token) +} diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 96cb9d19516f3..8e3e11d6ebae8 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -280,6 +280,17 @@ type OIDCConfig struct { UserRolesDefault clibase.StringArray `json:"user_roles_default" typescript:",notnull"` SignInText clibase.String `json:"sign_in_text" typescript:",notnull"` IconURL clibase.URL `json:"icon_url" typescript:",notnull"` + // OIDC Provider Specific Configurations + // Ideally all OIDC providers would follow all the same standards, but in practice they + // all have unique quirks. + +} + +// AzureADOIDCConfig is the configuration for Azure AD OIDC IDP. +// Try at all costs to keep configuration in the generic OIDCConfig struct above. +// Only use this if you absolutely need to. +type AzureADOIDCConfig struct { + Certificate clibase.String } type TelemetryConfig struct { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index c1aa96d872994..3237bfeacfebe 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -139,6 +139,11 @@ export interface AuthorizationRequest { // From codersdk/authorization.go export type AuthorizationResponse = Record +// From codersdk/deployment.go +export interface AzureADOIDCConfig { + readonly Certificate: string +} + // From codersdk/deployment.go export interface BuildInfoResponse { readonly external_url: string From c62e5485aca1dbd046ecb6c60acd91a85b22ec85 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Aug 2023 11:18:13 -0500 Subject: [PATCH 02/14] add client cert and key as deployment options --- cli/server.go | 39 +++++++++-- coderd/azureauth/oidcpki.go | 94 --------------------------- coderd/oauthpki/oidcpki.go | 115 +++++++++++++++++++++++++++++++++ codersdk/deployment.go | 38 +++++++---- site/src/api/typesGenerated.ts | 7 +- 5 files changed, 174 insertions(+), 119 deletions(-) delete mode 100644 coderd/azureauth/oidcpki.go create mode 100644 coderd/oauthpki/oidcpki.go diff --git a/cli/server.go b/cli/server.go index ad1742316b0ab..ab3ba519d8c33 100644 --- a/cli/server.go +++ b/cli/server.go @@ -33,7 +33,7 @@ import ( "sync/atomic" "time" - "github.com/coder/coder/coderd/azureauth" + "github.com/coder/coder/coderd/oauthpki" "github.com/coreos/go-oidc/v3/oidc" "github.com/coreos/go-systemd/daemon" @@ -553,7 +553,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. } } - if true || cfg.OIDC.ClientSecret != "" { + if cfg.OIDC.ClientKeyFile != "" || cfg.OIDC.ClientSecret != "" { if cfg.OIDC.ClientID == "" { return xerrors.Errorf("OIDC client ID be set!") } @@ -587,12 +587,21 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. Endpoint: oidcProvider.Endpoint(), Scopes: cfg.OIDC.Scopes, } - jwtCfg, err := azureauth.NewJWTAssertion(oauthCfg) - if err != nil { - return xerrors.Errorf("configure azure auth: %w", err) + + var useCfg httpmw.OAuth2Config = oauthCfg + if cfg.OIDC.ClientKeyFile != "" { + if cfg.OIDC.ClientSecret != "" { + return xerrors.Errorf("cannot specify both oidc client secret and oidc client key file") + } + + pkiCfg, err := configureOIDCPKI(oauthCfg, cfg.OIDC.ClientKeyFile.Value(), cfg.OIDC.ClientCertFile.Value()) + if err != nil { + return xerrors.Errorf("configure oauth pki authentication: %w", err) + } + useCfg = pkiCfg } options.OIDCConfig = &coderd.OIDCConfig{ - OAuth2Config: jwtCfg, + OAuth2Config: useCfg, Provider: oidcProvider, Verifier: oidcProvider.Verifier(&oidc.Config{ ClientID: cfg.OIDC.ClientID.String(), @@ -1501,6 +1510,24 @@ func configureTLS(tlsMinVersion, tlsClientAuth string, tlsCertFiles, tlsKeyFiles return tlsConfig, nil } +func configureOIDCPKI(orig *oauth2.Config, keyFile string, certFile string) (*oauthpki.Config, error) { + // Read the files + keyData, err := os.ReadFile(keyFile) + if err != nil { + return nil, xerrors.Errorf("read oidc client key file: %w", err) + } + + var certData []byte + if certFile != "" { + certData, err = os.ReadFile(certFile) + if err != nil { + return nil, xerrors.Errorf("read oidc client cert file: %w", err) + } + } + + return oauthpki.NewOauth2PKIConfig(orig, keyData, certData) +} + func configureCAPool(tlsClientCAFile string, tlsConfig *tls.Config) error { if tlsClientCAFile != "" { caPool := x509.NewCertPool() diff --git a/coderd/azureauth/oidcpki.go b/coderd/azureauth/oidcpki.go deleted file mode 100644 index 6db192bcc526c..0000000000000 --- a/coderd/azureauth/oidcpki.go +++ /dev/null @@ -1,94 +0,0 @@ -package azureauth - -import ( - "context" - "crypto/rsa" - "crypto/sha1" - "crypto/x509" - "encoding/base64" - "encoding/pem" - "time" - - "github.com/google/uuid" - - "github.com/golang-jwt/jwt/v4" - - "golang.org/x/xerrors" - - "golang.org/x/oauth2" -) - -// JWTAssertion is used by Azure AD when doing OIDC with a PKI cert instead of -// a client secret. -// https://learn.microsoft.com/en-us/azure/active-directory/develop/certificate-credentials -type JWTAssertion struct { - *oauth2.Config - - // ClientSecret is the private key of the PKI cert. - // Azure AD only supports RS256 signing algorithm. - clientKey *rsa.PrivateKey - // Base64url-encoded SHA-1 thumbprint of the X.509 certificate's DER encoding. - x5t string -} - -func NewJWTAssertion(config *oauth2.Config, pemEncodedKey string, pemEncodedCert string) (*JWTAssertion, error) { - rsaKey, err := DecodeKeyCertificate([]byte(pemEncodedKey)) - if err != nil { - return nil, err - } - - block, _ := pem.Decode([]byte(pemEncodedCert)) - hashed := sha1.Sum(block.Bytes) - - return &JWTAssertion{ - Config: config, - clientKey: rsaKey, - x5t: base64.StdEncoding.EncodeToString(hashed[:]), - }, nil -} - -// DecodeKeyCertificate decodes a PEM encoded PKI cert. -func DecodeKeyCertificate(pemEncoded []byte) (*rsa.PrivateKey, error) { - block, _ := pem.Decode(pemEncoded) - key, err := x509.ParsePKCS1PrivateKey(block.Bytes) - if err != nil { - return nil, xerrors.Errorf("failed to parse private key: %w", err) - } - - return key, nil -} - -func (ja *JWTAssertion) AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string { - return ja.Config.AuthCodeURL(state, opts...) -} - -func (ja *JWTAssertion) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { - now := time.Now() - token := jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ - "aud": ja.Config.Endpoint.TokenURL, - "exp": now.Add(time.Minute * 5).Unix(), - "jti": uuid.New().String(), - "nbf": now.Unix(), - "iat": now.Unix(), - - // TODO: Should be app GUID, not client ID. - "iss": ja.Config.ClientID, - "sub": ja.Config.ClientID, - }) - token.Header["x5t"] = ja.x5t - - signed, err := token.SignedString(ja.clientKey) - if err != nil { - return nil, xerrors.Errorf("failed to sign jwt assertion: %w", err) - } - - opts = append(opts, - oauth2.SetAuthURLParam("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"), - oauth2.SetAuthURLParam("client_assertion", signed), - ) - return ja.Config.Exchange(ctx, code, opts...) -} - -func (ja *JWTAssertion) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.TokenSource { - return ja.Config.TokenSource(ctx, token) -} diff --git a/coderd/oauthpki/oidcpki.go b/coderd/oauthpki/oidcpki.go new file mode 100644 index 0000000000000..5c14d695a15dc --- /dev/null +++ b/coderd/oauthpki/oidcpki.go @@ -0,0 +1,115 @@ +package oauthpki + +import ( + "context" + "crypto/rsa" + "crypto/sha1" + "crypto/x509" + "encoding/base64" + "encoding/pem" + "strings" + "time" + + "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" + "golang.org/x/oauth2" + "golang.org/x/xerrors" +) + +// Config uses jwt assertions over client_secret for oauth2 authentication of +// the application. This implementation was made specifically for Azure AD. +// +// https://learn.microsoft.com/en-us/azure/active-directory/develop/certificate-credentials +// +// However this does mostly follow the standard. We can generalize this as we +// include support for more IDPs. +// +// https://datatracker.ietf.org/doc/html/rfc7523 +type Config struct { + *oauth2.Config + + // ClientSecret is the private key of the PKI cert. + // Azure AD only supports RS256 signing algorithm. + clientKey *rsa.PrivateKey + // Base64url-encoded SHA-1 thumbprint of the X.509 certificate's DER encoding. + // This is specific to Azure AD + x5t string +} + +// NewOauth2PKIConfig creates the oauth2 config for PKI based auth. It requires the certificate and it's private key. +// The values should be passed in as PEM encoded values, which is the standard encoding for x509 certs saved to disk. +// It should look like: +// +// -----BEGIN RSA PRIVATE KEY---- +// ... +// -----END RSA PRIVATE KEY----- +// +// -----BEGIN CERTIFICATE----- +// ... +// -----END CERTIFICATE----- +func NewOauth2PKIConfig(config *oauth2.Config, pemEncodedKey []byte, pemEncodedCert []byte) (*Config, error) { + rsaKey, err := decodeKeyCertificate(pemEncodedKey) + if err != nil { + return nil, err + } + + // Azure AD requires a certificate. The sha1 of the cert is used to identify the signer. + // This is not required in the general specification. + if strings.Contains(strings.ToLower(config.Endpoint.TokenURL), "microsoftonline") && len(pemEncodedCert) == 0 { + return nil, xerrors.Errorf("oidc client certificate is required and missing") + } + + block, _ := pem.Decode(pemEncodedCert) + hashed := sha1.Sum(block.Bytes) + + return &Config{ + Config: config, + clientKey: rsaKey, + x5t: base64.StdEncoding.EncodeToString(hashed[:]), + }, nil +} + +// decodeKeyCertificate decodes a PEM encoded PKI cert. +func decodeKeyCertificate(pemEncoded []byte) (*rsa.PrivateKey, error) { + block, _ := pem.Decode(pemEncoded) + key, err := x509.ParsePKCS1PrivateKey(block.Bytes) + if err != nil { + return nil, xerrors.Errorf("failed to parse private key: %w", err) + } + + return key, nil +} + +func (ja *Config) AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string { + return ja.Config.AuthCodeURL(state, opts...) +} + +// Exchange includes the client_assertion signed JWT. +func (ja *Config) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { + now := time.Now() + token := jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "iss": ja.Config.ClientID, + "sub": ja.Config.ClientID, + "aud": ja.Config.Endpoint.TokenURL, + "exp": now.Add(time.Minute * 5).Unix(), + "jti": uuid.New().String(), + "nbf": now.Unix(), + "iat": now.Unix(), + }) + token.Header["x5t"] = ja.x5t + + signed, err := token.SignedString(ja.clientKey) + if err != nil { + return nil, xerrors.Errorf("failed to sign jwt assertion: %w", err) + } + + opts = append(opts, + oauth2.SetAuthURLParam("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"), + oauth2.SetAuthURLParam("client_assertion", signed), + ) + return ja.Config.Exchange(ctx, code, opts...) +} + +func (ja *Config) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.TokenSource { + return ja.Config.TokenSource(ctx, token) +} diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 8e3e11d6ebae8..8b80c750224b4 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -260,9 +260,12 @@ type OAuth2GithubConfig struct { } type OIDCConfig struct { - AllowSignups clibase.Bool `json:"allow_signups" typescript:",notnull"` - ClientID clibase.String `json:"client_id" typescript:",notnull"` - ClientSecret clibase.String `json:"client_secret" typescript:",notnull"` + AllowSignups clibase.Bool `json:"allow_signups" typescript:",notnull"` + ClientID clibase.String `json:"client_id" typescript:",notnull"` + ClientSecret clibase.String `json:"client_secret" typescript:",notnull"` + // ClientKeyFile & ClientCertFile are used in place of ClientSecret for PKI auth. + ClientKeyFile clibase.String `json:"client_key_file" typescript:",notnull"` + ClientCertFile clibase.String `json:"client_cert_file" typescript:",notnull"` EmailDomain clibase.StringArray `json:"email_domain" typescript:",notnull"` IssuerURL clibase.String `json:"issuer_url" typescript:",notnull"` Scopes clibase.StringArray `json:"scopes" typescript:",notnull"` @@ -280,17 +283,6 @@ type OIDCConfig struct { UserRolesDefault clibase.StringArray `json:"user_roles_default" typescript:",notnull"` SignInText clibase.String `json:"sign_in_text" typescript:",notnull"` IconURL clibase.URL `json:"icon_url" typescript:",notnull"` - // OIDC Provider Specific Configurations - // Ideally all OIDC providers would follow all the same standards, but in practice they - // all have unique quirks. - -} - -// AzureADOIDCConfig is the configuration for Azure AD OIDC IDP. -// Try at all costs to keep configuration in the generic OIDCConfig struct above. -// Only use this if you absolutely need to. -type AzureADOIDCConfig struct { - Certificate clibase.String } type TelemetryConfig struct { @@ -977,6 +969,24 @@ when required by your organization's security policy.`, Value: &c.OIDC.ClientSecret, Group: &deploymentGroupOIDC, }, + { + Name: "OIDC Client Key File", + Description: "Pem encoded RSA private key to use for oauth2 PKI/JWT authorization. " + + "This can be used instead of oidc-client-secret if your IDP supports it.", + Flag: "oidc-client-key-file", + Env: "CODER_OIDC_CLIENT_KEY_FILE", + Value: &c.OIDC.ClientKeyFile, + Group: &deploymentGroupOIDC, + }, + { + Name: "OIDC Client Cert File", + Description: "Pem encoded certificate file to use for oauth2 PKI/JWT authorization. " + + "The public certificate that accompanies oidc-client-key-file. A standard x509 certificate is expected.", + Flag: "oidc-client-cert-file", + Env: "CODER_OIDC_CLIENT_CERT_FILE", + Value: &c.OIDC.ClientCertFile, + Group: &deploymentGroupOIDC, + }, { Name: "OIDC Email Domain", Description: "Email domains that clients logging in with OIDC must match.", diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 3237bfeacfebe..a8119056ffbbf 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -139,11 +139,6 @@ export interface AuthorizationRequest { // From codersdk/authorization.go export type AuthorizationResponse = Record -// From codersdk/deployment.go -export interface AzureADOIDCConfig { - readonly Certificate: string -} - // From codersdk/deployment.go export interface BuildInfoResponse { readonly external_url: string @@ -620,6 +615,8 @@ export interface OIDCConfig { readonly allow_signups: boolean readonly client_id: string readonly client_secret: string + readonly client_key_file: string + readonly client_cert_file: string // This is likely an enum in an external package ("github.com/coder/coder/cli/clibase.StringArray") readonly email_domain: string[] readonly issuer_url: string From 664895f387e91e7b3cb9b70273a9bfeeee1295ac Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Aug 2023 12:29:52 -0500 Subject: [PATCH 03/14] add unit test --- cli/server.go | 11 ++- coderd/oauthpki/oidcpki.go | 46 ++++++--- coderd/oauthpki/okidcpki_test.go | 155 +++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 17 deletions(-) create mode 100644 coderd/oauthpki/okidcpki_test.go diff --git a/cli/server.go b/cli/server.go index ab3ba519d8c33..e52aa402d158f 100644 --- a/cli/server.go +++ b/cli/server.go @@ -33,8 +33,6 @@ import ( "sync/atomic" "time" - "github.com/coder/coder/coderd/oauthpki" - "github.com/coreos/go-oidc/v3/oidc" "github.com/coreos/go-systemd/daemon" embeddedpostgres "github.com/fergusstrange/embedded-postgres" @@ -78,6 +76,7 @@ import ( "github.com/coder/coder/coderd/gitsshkey" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/oauthpki" "github.com/coder/coder/coderd/prometheusmetrics" "github.com/coder/coder/coderd/schedule" "github.com/coder/coder/coderd/telemetry" @@ -1525,7 +1524,13 @@ func configureOIDCPKI(orig *oauth2.Config, keyFile string, certFile string) (*oa } } - return oauthpki.NewOauth2PKIConfig(orig, keyData, certData) + return oauthpki.NewOauth2PKIConfig(oauthpki.ConfigParams{ + ClientID: orig.ClientID, + TokenURL: orig.Endpoint.TokenURL, + PemEncodedKey: keyData, + PemEncodedCert: certData, + Config: orig, + }) } func configureCAPool(tlsClientCAFile string, tlsConfig *tls.Config) error { diff --git a/coderd/oauthpki/oidcpki.go b/coderd/oauthpki/oidcpki.go index 5c14d695a15dc..af6b06ff3beee 100644 --- a/coderd/oauthpki/oidcpki.go +++ b/coderd/oauthpki/oidcpki.go @@ -10,6 +10,8 @@ import ( "strings" "time" + "github.com/coder/coder/coderd/httpmw" + "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "golang.org/x/oauth2" @@ -26,8 +28,10 @@ import ( // // https://datatracker.ietf.org/doc/html/rfc7523 type Config struct { - *oauth2.Config + cfg httpmw.OAuth2Config + clientID string + tokenURL string // ClientSecret is the private key of the PKI cert. // Azure AD only supports RS256 signing algorithm. clientKey *rsa.PrivateKey @@ -36,6 +40,15 @@ type Config struct { x5t string } +type ConfigParams struct { + ClientID string + TokenURL string + PemEncodedKey []byte + PemEncodedCert []byte + + Config httpmw.OAuth2Config +} + // NewOauth2PKIConfig creates the oauth2 config for PKI based auth. It requires the certificate and it's private key. // The values should be passed in as PEM encoded values, which is the standard encoding for x509 certs saved to disk. // It should look like: @@ -47,30 +60,35 @@ type Config struct { // -----BEGIN CERTIFICATE----- // ... // -----END CERTIFICATE----- -func NewOauth2PKIConfig(config *oauth2.Config, pemEncodedKey []byte, pemEncodedCert []byte) (*Config, error) { - rsaKey, err := decodeKeyCertificate(pemEncodedKey) +func NewOauth2PKIConfig(params ConfigParams) (*Config, error) { + if params.ClientID == "" { + return nil, xerrors.Errorf("") + } + rsaKey, err := decodeClientKey(params.PemEncodedKey) if err != nil { return nil, err } // Azure AD requires a certificate. The sha1 of the cert is used to identify the signer. // This is not required in the general specification. - if strings.Contains(strings.ToLower(config.Endpoint.TokenURL), "microsoftonline") && len(pemEncodedCert) == 0 { + if strings.Contains(strings.ToLower(params.TokenURL), "microsoftonline") && len(params.PemEncodedCert) == 0 { return nil, xerrors.Errorf("oidc client certificate is required and missing") } - block, _ := pem.Decode(pemEncodedCert) + block, _ := pem.Decode(params.PemEncodedCert) hashed := sha1.Sum(block.Bytes) return &Config{ - Config: config, + clientID: params.ClientID, + tokenURL: params.TokenURL, + cfg: params.Config, clientKey: rsaKey, x5t: base64.StdEncoding.EncodeToString(hashed[:]), }, nil } -// decodeKeyCertificate decodes a PEM encoded PKI cert. -func decodeKeyCertificate(pemEncoded []byte) (*rsa.PrivateKey, error) { +// decodeClientKey decodes a PEM encoded rsa secret. +func decodeClientKey(pemEncoded []byte) (*rsa.PrivateKey, error) { block, _ := pem.Decode(pemEncoded) key, err := x509.ParsePKCS1PrivateKey(block.Bytes) if err != nil { @@ -81,16 +99,16 @@ func decodeKeyCertificate(pemEncoded []byte) (*rsa.PrivateKey, error) { } func (ja *Config) AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) string { - return ja.Config.AuthCodeURL(state, opts...) + return ja.cfg.AuthCodeURL(state, opts...) } // Exchange includes the client_assertion signed JWT. func (ja *Config) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { now := time.Now() token := jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ - "iss": ja.Config.ClientID, - "sub": ja.Config.ClientID, - "aud": ja.Config.Endpoint.TokenURL, + "iss": ja.clientID, + "sub": ja.clientID, + "aud": ja.tokenURL, "exp": now.Add(time.Minute * 5).Unix(), "jti": uuid.New().String(), "nbf": now.Unix(), @@ -107,9 +125,9 @@ func (ja *Config) Exchange(ctx context.Context, code string, opts ...oauth2.Auth oauth2.SetAuthURLParam("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"), oauth2.SetAuthURLParam("client_assertion", signed), ) - return ja.Config.Exchange(ctx, code, opts...) + return ja.cfg.Exchange(ctx, code, opts...) } func (ja *Config) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.TokenSource { - return ja.Config.TokenSource(ctx, token) + return ja.cfg.TokenSource(ctx, token) } diff --git a/coderd/oauthpki/okidcpki_test.go b/coderd/oauthpki/okidcpki_test.go new file mode 100644 index 0000000000000..ecc6095b66ebc --- /dev/null +++ b/coderd/oauthpki/okidcpki_test.go @@ -0,0 +1,155 @@ +package oauthpki_test + +import ( + "context" + "encoding/base64" + "io" + "net/http" + "net/url" + "testing" + + "github.com/coreos/go-oidc/v3/oidc" + "github.com/golang-jwt/jwt" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + + "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/oauthpki" + "github.com/coder/coder/testutil" +) + +const ( + testClientKey = `-----BEGIN RSA PRIVATE KEY----- +MIIEpAIBAAKCAQEAnUryZEfn5kA8wuk9a7ogFuWbk3uPHEhioYuAg9m3/tIdqSqu +ASpRzw8+1nORTf3ykWRRlhxZWnKimmkB0Ux5Yrz9TDVWDQbzEH3B8ibMlmaNcoN8 +wYVzeEpqCe3fJagnV0lh0sHB1Z+vhcJ/M2nEAdyfhIgQEbG6Xtl2+WcGqyMWUJpV +g8+ebK+JkXELAGN1hg3DdV52gjodEjoe1/ibHz8y3NR7j2tOKix7iKOhccyFkD35 +xqSnfyZJK5yxIfmGiWdVOIGqc2rYpgvrXJLTOjLoeyDSNi+Q604T64ZxsqfuM4LX +BakVG3EwHFXPBfsBKjUE9HYvXEXw3fJP9K6mIwIDAQABAoIBAQCb+aH7x0IylSir +r1Z06RDBI9bunOwBA9aqkwdRuCg4zGsVQXljNnABgACz7837JQPRIUW2MU553otX +yyE+RzNnsjkLxSgbqvSFOe+FDOx7iB5jm/euf4NNmZ0lU3iggurgJ6iVsgVgrQUF +AyXX+d2gawLUDYjBwxgozkSodH2sXYSX+SWfSOXHsFzSa3tLtUMbAIflM0rlRXf7 +Z57M8mMomZUvmmojH+TnBQljJlU8lhrvOaDD4DT8qAtVHE3VluDBQ9/3E8OIjz+E +EqUgWLgrdq1rIMhJbHN90NwLwWs+2PcRfdB6hqKPktLne2KZFOgVKlxPKOYByBq1 +PX/vJ/HBAoGBAMFmJ6nYqyUVl26ajlXmnXBjQ+iBLHo9lcUu84+rpqRf90Bsm5bd +jMmYr3Yo3yXNiit3rvZzBfPElo+IVa1HpPtgOaa2AU5B3QzxWCNT0FNRQqMG2LcA +CvB10pOdJEABQxr7d4eFRg2/KbF1fr0r0vqMEelwa5ejTg6ROD3DtadpAoGBANA0 +4EClniCwvd1IECy2oTuTDosXgmRKwRAcwgE34YXy1Y/L4X/ghFeCHi3ybrep0uwL +ptJNK+0sqvPu6UhC356GfMqfuzOKNMkXybnPUbHrz5KTkN+QQMfPc73Veel2gpD3 +xNataEmHtxcOx0X1OnjwyZZpmMbrUY3Cackn+durAoGBAKYR5nU+jJfnloVvSlIR +GZhsZN++LEc7ouQTkSoJp6r2jQZRPLmrvT1PUzwPlK6NdNwmhaMy2iWc5fySgZ+u +KcmBs3+oQi7E9+ApThnn2rfwy1vagTWDX+FkC1KeWYZsjwcYcGd61dDwGgk8b3xZ +qW1j4e2mj31CycBQiw7eg5ohAoGADvkOe3etlHpBXS12hFCp7afYruYE6YN6uNbo +mL/VBxX8h7fIwrJ5sfVYiENb9PdQhMsdtxf3pbnFnX875Ydxn2vag5PTGZTB0QhV +6HfhTyM/LTJRg9JS5kuj7i3w83ojT5uR20JjMo6A+zaD3CMTjmj6hkeXxg5cMg6e +HuoyDLsCgYBcbboYMFT1cUSxBeMtPGt3CxxZUYnUQaRUeOcjqYYlFL+DCWhY7pxH +EnLhwW/KzkDzOmwRmmNOMqD7UhR/ayxR+avRt6v5d5l8fVCuNexgs7kR9L5IQp9l +YV2wsCoXBCcuPmio/te44U//BlzprEu0w1iHpb3ibmQg4y291R0TvQ== +-----END RSA PRIVATE KEY-----` + + testClientCert = ` +-----BEGIN CERTIFICATE----- +MIIEOjCCAiKgAwIBAgIQMO50KnWsRbmrrthPQgyubjANBgkqhkiG9w0BAQsFADAY +MRYwFAYDVQQDEw1Mb2NhbGhvc3RDZXJ0MB4XDTIzMDgxMDE2MjYxOFoXDTI1MDIx +MDE2MjU0M1owFDESMBAGA1UEAxMJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEF +AAOCAQ8AMIIBCgKCAQEAnUryZEfn5kA8wuk9a7ogFuWbk3uPHEhioYuAg9m3/tId +qSquASpRzw8+1nORTf3ykWRRlhxZWnKimmkB0Ux5Yrz9TDVWDQbzEH3B8ibMlmaN +coN8wYVzeEpqCe3fJagnV0lh0sHB1Z+vhcJ/M2nEAdyfhIgQEbG6Xtl2+WcGqyMW +UJpVg8+ebK+JkXELAGN1hg3DdV52gjodEjoe1/ibHz8y3NR7j2tOKix7iKOhccyF +kD35xqSnfyZJK5yxIfmGiWdVOIGqc2rYpgvrXJLTOjLoeyDSNi+Q604T64Zxsqfu +M4LXBakVG3EwHFXPBfsBKjUE9HYvXEXw3fJP9K6mIwIDAQABo4GDMIGAMA4GA1Ud +DwEB/wQEAwIDuDAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwHQYDVR0O +BBYEFAYCdgydG3h2SNWF+BfAyJtNliJtMB8GA1UdIwQYMBaAFHR/aptP0RUNNFyf +5uky527SECt1MA8GA1UdEQQIMAaHBH8AAAEwDQYJKoZIhvcNAQELBQADggIBAI6P +ymG7l06JvJ3p6xgaMyOxgkpQl6WkY4LJHVEhfeDSoO3qsJc4PxUdSExJsT84weXb +lF+tK6D/CPlvjmG720IlB5cSKJ71rWjwmaMWKxWKXyoZdDrHAp55+FNdXegUZF2o +EF/ZM5CHaO8iHMkuWEv1OASHBQWC/o4spUN5HGQ9HepwLVvO/aX++LYfvfL9faKA +IT+w9i8pJbfItFmfA8x2OEVZk8aEA0WtKdfsMwzGmZ1GSGa4UYcynxQGCMiB5h4L +C/dpoJRbEzdGLuTZgV2SCaN3k5BrH4aaILI9tqZaq0gamN9Rd2yji3cGiduCeAAo +RmVcl9fBliMLxylWEP5+B2JmCZEc8Lfm0TBNnjaG17KY40gzbfBYixBxBTYgsPua +bfprtfksSG++zcsDbkC8CtPamtlNWtDAiFp4yQRkP79PlJO6qCdTrFWPukTMCMso +25hjLvxj1fLy/jSMDEZu/oQ14TMCZSGHRjz4CPiaCfXqgqOtVOD+5+yWInwUGp/i +Nb1vIq4ruEAbyCbdWKHbE0yT5AP7hm5ZNybpZ4/311AEBD2HKip/OqB05p99XcLw +BIC4ODNvwCn6x00KZoqWz/MX2dEQ/HqWiWaDB/OSemfTVE3I94mzEWnqpF2cQpcT +B1B7CpkMU55hPP+7nsofCszNrMDXT8Z5w2a3zLKM +-----END CERTIFICATE----- +` +) + +type exchangeAssert struct { + httpmw.OAuth2Config + assert func(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) +} + +func (a *exchangeAssert) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { + a.assert(ctx, code, opts...) + return a.OAuth2Config.Exchange(ctx, code, opts...) +} + +type fakeRoundTripper struct { + roundTrip func(req *http.Request) (*http.Response, error) +} + +func (f fakeRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + return f.roundTrip(req) +} + +// TestAzureADPKIOIDC ensures we do not break Azure AD compatibility. +func TestAzureADPKIOIDC(t *testing.T) { + oauthCfg := &oauth2.Config{ + ClientID: "random-client-id", + Endpoint: oauth2.Endpoint{ + TokenURL: "https://login.microsoftonline.com/6a1e9139-13f2-4afb-8f46-036feac8bd79/v2.0/token", + }, + } + + pkiConfig, err := oauthpki.NewOauth2PKIConfig(oauthpki.ConfigParams{ + ClientID: oauthCfg.ClientID, + TokenURL: oauthCfg.Endpoint.TokenURL, + PemEncodedKey: []byte(testClientKey), + PemEncodedCert: []byte(testClientCert), + Config: oauthCfg, + }) + require.NoError(t, err, "failed to create pki config") + + ctx := testutil.Context(t, testutil.WaitMedium) + ctx = oidc.ClientContext(ctx, &http.Client{ + Transport: &fakeRoundTripper{ + roundTrip: func(req *http.Request) (*http.Response, error) { + resp := &http.Response{ + Status: "500 Internal Service Error", + } + // This is the easiest way to hijack the request and check + // the params. The oauth2 package uses unexported types and + // options, so we need to view the actual request created. + data, err := io.ReadAll(req.Body) + if !assert.NoError(t, err, "failed to read request body") { + return resp, nil + } + vals, err := url.ParseQuery(string(data)) + if !assert.NoError(t, err, "failed to parse values") { + return resp, nil + } + assert.Equal(t, "urn:ietf:params:oauth:client-assertion-type:jwt-bearer", vals.Get("client_assertion_type")) + + jwtToken := vals.Get("client_assertion") + + // No need to actually verify the jwt is signed right. + parsedToken, _, err := (&jwt.Parser{}).ParseUnverified(jwtToken, jwt.MapClaims{}) + if !assert.NoError(t, err, "failed to parse jwt token") { + return resp, nil + } + + // Azure requirements + assert.NotEmpty(t, parsedToken.Header["x5t"], "hashed cert missing") + assert.Equal(t, "RS256", parsedToken.Header["alg"], "azure only accepts RS256") + assert.Equal(t, "JWT", parsedToken.Header["typ"], "azure only accepts JWT") + return resp, nil + }, + }, + }) + token, err := pkiConfig.Exchange(ctx, base64.StdEncoding.EncodeToString([]byte("random-code"))) + // We hijack the request and return an error intentionally + require.Error(t, err, "error expected") +} From 74f92451edc8bedf78463c74fa7a1aa664bfb7e2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Aug 2023 12:30:48 -0500 Subject: [PATCH 04/14] fixup! add unit test --- coderd/oauthpki/okidcpki_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/oauthpki/okidcpki_test.go b/coderd/oauthpki/okidcpki_test.go index ecc6095b66ebc..d5cdb4c9cc443 100644 --- a/coderd/oauthpki/okidcpki_test.go +++ b/coderd/oauthpki/okidcpki_test.go @@ -149,7 +149,7 @@ func TestAzureADPKIOIDC(t *testing.T) { }, }, }) - token, err := pkiConfig.Exchange(ctx, base64.StdEncoding.EncodeToString([]byte("random-code"))) + _, err = pkiConfig.Exchange(ctx, base64.StdEncoding.EncodeToString([]byte("random-code"))) // We hijack the request and return an error intentionally require.Error(t, err, "error expected") } From ce39f2daca178ecff31641a13490ce96b7138c5c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Aug 2023 13:58:10 -0500 Subject: [PATCH 05/14] Refactor code into a method --- cli/server.go | 4 ++++ coderd/oauthpki/oidcpki.go | 22 +++++++++++++++------- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/cli/server.go b/cli/server.go index e52aa402d158f..0edcb1b785020 100644 --- a/cli/server.go +++ b/cli/server.go @@ -589,6 +589,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. var useCfg httpmw.OAuth2Config = oauthCfg if cfg.OIDC.ClientKeyFile != "" { + // PKI authentication is done in the params. If a + // counter example is found, we can add a config option to + // change this. + oauthCfg.Endpoint.AuthStyle = oauth2.AuthStyleInParams if cfg.OIDC.ClientSecret != "" { return xerrors.Errorf("cannot specify both oidc client secret and oidc client key file") } diff --git a/coderd/oauthpki/oidcpki.go b/coderd/oauthpki/oidcpki.go index af6b06ff3beee..11d417538d31c 100644 --- a/coderd/oauthpki/oidcpki.go +++ b/coderd/oauthpki/oidcpki.go @@ -104,6 +104,18 @@ func (ja *Config) AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) strin // Exchange includes the client_assertion signed JWT. func (ja *Config) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { + signed, err := ja.jwtToken() + if err != nil { + return nil, xerrors.Errorf("failed jwt assertion: %w", err) + } + opts = append(opts, + oauth2.SetAuthURLParam("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"), + oauth2.SetAuthURLParam("client_assertion", signed), + ) + return ja.cfg.Exchange(ctx, code, opts...) +} + +func (ja *Config) jwtToken() (string, error) { now := time.Now() token := jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ "iss": ja.clientID, @@ -118,16 +130,12 @@ func (ja *Config) Exchange(ctx context.Context, code string, opts ...oauth2.Auth signed, err := token.SignedString(ja.clientKey) if err != nil { - return nil, xerrors.Errorf("failed to sign jwt assertion: %w", err) + return "", xerrors.Errorf("sign jwt assertion: %w", err) } - - opts = append(opts, - oauth2.SetAuthURLParam("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"), - oauth2.SetAuthURLParam("client_assertion", signed), - ) - return ja.cfg.Exchange(ctx, code, opts...) + return signed, nil } func (ja *Config) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.TokenSource { + // TODO: Hijack the http.Client to insert proper client auth assertions. return ja.cfg.TokenSource(ctx, token) } From 7346fbfc7aba7084dde048eac241bb6039b8ba96 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Aug 2023 16:33:20 -0500 Subject: [PATCH 06/14] Custom token refresher to handle pki auth --- cli/server.go | 1 + coderd/oauthpki/oidcpki.go | 126 +++++++++++++++++++++++++++++++++++-- 2 files changed, 123 insertions(+), 4 deletions(-) diff --git a/cli/server.go b/cli/server.go index 0edcb1b785020..61fbe89e2e049 100644 --- a/cli/server.go +++ b/cli/server.go @@ -1531,6 +1531,7 @@ func configureOIDCPKI(orig *oauth2.Config, keyFile string, certFile string) (*oa return oauthpki.NewOauth2PKIConfig(oauthpki.ConfigParams{ ClientID: orig.ClientID, TokenURL: orig.Endpoint.TokenURL, + Scopes: orig.Scopes, PemEncodedKey: keyData, PemEncodedCert: certData, Config: orig, diff --git a/coderd/oauthpki/oidcpki.go b/coderd/oauthpki/oidcpki.go index 11d417538d31c..1f39ac07eaee3 100644 --- a/coderd/oauthpki/oidcpki.go +++ b/coderd/oauthpki/oidcpki.go @@ -6,16 +6,22 @@ import ( "crypto/sha1" "crypto/x509" "encoding/base64" + "encoding/json" "encoding/pem" + "fmt" + "io" + "net/http" + "net/url" "strings" "time" - "github.com/coder/coder/coderd/httpmw" - "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "golang.org/x/oauth2" + "golang.org/x/oauth2/jws" "golang.org/x/xerrors" + + "github.com/coder/coder/coderd/httpmw" ) // Config uses jwt assertions over client_secret for oauth2 authentication of @@ -30,6 +36,7 @@ import ( type Config struct { cfg httpmw.OAuth2Config + scopes []string clientID string tokenURL string // ClientSecret is the private key of the PKI cert. @@ -43,6 +50,7 @@ type Config struct { type ConfigParams struct { ClientID string TokenURL string + Scopes []string PemEncodedKey []byte PemEncodedCert []byte @@ -64,6 +72,10 @@ func NewOauth2PKIConfig(params ConfigParams) (*Config, error) { if params.ClientID == "" { return nil, xerrors.Errorf("") } + if len(params.Scopes) == 0 { + return nil, xerrors.Errorf("scopes are required") + } + rsaKey, err := decodeClientKey(params.PemEncodedKey) if err != nil { return nil, err @@ -81,6 +93,7 @@ func NewOauth2PKIConfig(params ConfigParams) (*Config, error) { return &Config{ clientID: params.ClientID, tokenURL: params.TokenURL, + scopes: params.Scopes, cfg: params.Config, clientKey: rsaKey, x5t: base64.StdEncoding.EncodeToString(hashed[:]), @@ -136,6 +149,111 @@ func (ja *Config) jwtToken() (string, error) { } func (ja *Config) TokenSource(ctx context.Context, token *oauth2.Token) oauth2.TokenSource { - // TODO: Hijack the http.Client to insert proper client auth assertions. - return ja.cfg.TokenSource(ctx, token) + return oauth2.ReuseTokenSource(token, &jwtTokenSource{ + cfg: ja, + ctx: ctx, + refreshToken: token.RefreshToken, + }) +} + +type jwtTokenSource struct { + cfg *Config + ctx context.Context + refreshToken string +} + +// Token must be safe for concurrent use by multiple go routines +// Very similar to the RetrieveToken implementation by the oauth2 package. +// https://github.com/golang/oauth2/blob/master/internal/token.go#L212 +// Oauth2 package keeps this code unexported or in an /internal package, +// so we have to copy the implementation :( +func (src *jwtTokenSource) Token() (*oauth2.Token, error) { + if src.refreshToken == "" { + return nil, xerrors.New("oauth2: token expired and refresh token is not set") + } + cli := http.DefaultClient + if v, ok := src.ctx.Value(oauth2.HTTPClient).(*http.Client); ok { + cli = v + } + + token, err := src.cfg.jwtToken() + if err != nil { + return nil, xerrors.Errorf("failed jwt assertion: %w", err) + } + + v := url.Values{ + "client_assertion": {token}, + "client_assertion_type": {"urn:ietf:params:oauth:client-assertion-type:jwt-bearer"}, + "client_id": {src.cfg.clientID}, + "grant_type": {"refresh_token"}, + "scope": {strings.Join(src.cfg.scopes, " ")}, + "refresh_token": {src.refreshToken}, + } + // Using params based auth + resp, err := cli.PostForm(src.cfg.tokenURL, v) + if err != nil { + return nil, xerrors.Errorf("oauth2: cannot get token: %w", err) + } + + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, xerrors.Errorf("oauth2: cannot fetch token reading response body: %w", err) + } + + var tokenRes struct { + oauth2.Token + // Extra fields returned by the refresh that are needed + IDToken string `json:"id_token"` + ExpiresIn int64 `json:"expires_in"` // relative seconds from now + // error fields + // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 + ErrorCode string `json:"error"` + ErrorDescription string `json:"error_description"` + ErrorURI string `json:"error_uri"` + } + + unmarshalError := json.Unmarshal(body, &tokenRes) + + if resp.StatusCode < 200 || resp.StatusCode > 299 { + // Return a standard oauth2 error. Attempt to read some error fields. The error fields + // can be encoded in a few places, so this does not catch all of them. + return nil, &oauth2.RetrieveError{ + Response: resp, + Body: body, + // Best effort for error fields + ErrorCode: tokenRes.ErrorCode, + ErrorDescription: tokenRes.ErrorDescription, + ErrorURI: tokenRes.ErrorURI, + } + } + + if unmarshalError != nil { + return nil, fmt.Errorf("oauth2: cannot unmarshal token: %v", err) + } + + newToken := &oauth2.Token{ + AccessToken: tokenRes.AccessToken, + TokenType: tokenRes.TokenType, + RefreshToken: tokenRes.RefreshToken, + } + + if secs := tokenRes.ExpiresIn; secs > 0 { + newToken.Expiry = time.Now().Add(time.Duration(secs) * time.Second) + } + + // ID token is a JWT token. We can decode it to get the expiry. + // Not really sure what to do if the ExpiresIn and JWT expiry differ, + // but this one is attached in the JWT and guaranteed to be right for local + // validation. So use this one if found. + if v := tokenRes.IDToken; v != "" { + // decode returned id token to get expiry + claimSet, err := jws.Decode(v) + if err != nil { + return nil, fmt.Errorf("oauth2: error decoding JWT token: %v", err) + } + newToken.Expiry = time.Unix(claimSet.Exp, 0) + } + + return newToken, nil } From 9c73346b62c3db0f94464da417a1411686d9295c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Aug 2023 18:01:17 -0500 Subject: [PATCH 07/14] Add unit test for mock e2e --- coderd/oauthpki/okidcpki_test.go | 215 +++++++++++++++++++++++++------ 1 file changed, 174 insertions(+), 41 deletions(-) diff --git a/coderd/oauthpki/okidcpki_test.go b/coderd/oauthpki/okidcpki_test.go index d5cdb4c9cc443..d7d45a27dad3f 100644 --- a/coderd/oauthpki/okidcpki_test.go +++ b/coderd/oauthpki/okidcpki_test.go @@ -5,16 +5,19 @@ import ( "encoding/base64" "io" "net/http" + "net/http/httptest" "net/url" + "strings" "testing" + "time" "github.com/coreos/go-oidc/v3/oidc" "github.com/golang-jwt/jwt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/oauth2" + "golang.org/x/xerrors" - "github.com/coder/coder/coderd/httpmw" "github.com/coder/coder/coderd/oauthpki" "github.com/coder/coder/testutil" ) @@ -77,25 +80,9 @@ B1B7CpkMU55hPP+7nsofCszNrMDXT8Z5w2a3zLKM ` ) -type exchangeAssert struct { - httpmw.OAuth2Config - assert func(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) -} - -func (a *exchangeAssert) Exchange(ctx context.Context, code string, opts ...oauth2.AuthCodeOption) (*oauth2.Token, error) { - a.assert(ctx, code, opts...) - return a.OAuth2Config.Exchange(ctx, code, opts...) -} - -type fakeRoundTripper struct { - roundTrip func(req *http.Request) (*http.Response, error) -} - -func (f fakeRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { - return f.roundTrip(req) -} - // TestAzureADPKIOIDC ensures we do not break Azure AD compatibility. +// It runs an oauth2.Exchange method and hijacks the request to only check the +// request side of the transaction. func TestAzureADPKIOIDC(t *testing.T) { oauthCfg := &oauth2.Config{ ClientID: "random-client-id", @@ -123,28 +110,7 @@ func TestAzureADPKIOIDC(t *testing.T) { // This is the easiest way to hijack the request and check // the params. The oauth2 package uses unexported types and // options, so we need to view the actual request created. - data, err := io.ReadAll(req.Body) - if !assert.NoError(t, err, "failed to read request body") { - return resp, nil - } - vals, err := url.ParseQuery(string(data)) - if !assert.NoError(t, err, "failed to parse values") { - return resp, nil - } - assert.Equal(t, "urn:ietf:params:oauth:client-assertion-type:jwt-bearer", vals.Get("client_assertion_type")) - - jwtToken := vals.Get("client_assertion") - - // No need to actually verify the jwt is signed right. - parsedToken, _, err := (&jwt.Parser{}).ParseUnverified(jwtToken, jwt.MapClaims{}) - if !assert.NoError(t, err, "failed to parse jwt token") { - return resp, nil - } - - // Azure requirements - assert.NotEmpty(t, parsedToken.Header["x5t"], "hashed cert missing") - assert.Equal(t, "RS256", parsedToken.Header["alg"], "azure only accepts RS256") - assert.Equal(t, "JWT", parsedToken.Header["typ"], "azure only accepts JWT") + assertJWTAuth(t, req) return resp, nil }, }, @@ -153,3 +119,170 @@ func TestAzureADPKIOIDC(t *testing.T) { // We hijack the request and return an error intentionally require.Error(t, err, "error expected") } + +// TestSavedAzureADPKIOIDC was created by capturing actual responses from an Azure +// AD instance and saving them to replay, removing some details. +// The reason this is done is that this is the only way to assert values +// passed to the oauth2 provider via http requests. +// It is not feasible to run against an actual Azure AD instance, so this attempts +// to prevent some regressions by running a full "e2e" oauth and asserting some +// of the request values. +func TestSavedAzureADPKIOIDC(t *testing.T) { + var ( + stateString = "random-state" + oauth2Code = base64.StdEncoding.EncodeToString([]byte("random-code")) + ) + + // Real oauth config. We will hijack all http requests so some of these values + // are fake. + cfg := &oauth2.Config{ + ClientID: "fake_app", + ClientSecret: "", + Endpoint: oauth2.Endpoint{ + AuthURL: "https://login.microsoftonline.com/fake_app/oauth2/v2.0/authorize", + TokenURL: "https://login.microsoftonline.com/fake_app/oauth2/v2.0/token", + AuthStyle: 0, + }, + RedirectURL: "http://localhost/api/v2/users/oidc/callback", + Scopes: []string{"openid", "profile", "email", "offline_access"}, + } + + initialExchange := false + tokenRefreshed := false + + // Create the oauthpki config + pki, err := oauthpki.NewOauth2PKIConfig(oauthpki.ConfigParams{ + ClientID: cfg.ClientID, + TokenURL: cfg.Endpoint.TokenURL, + Scopes: []string{"openid", "email", "profile", "offline_access"}, + PemEncodedKey: []byte(testClientKey), + PemEncodedCert: []byte(testClientCert), + Config: cfg, + }) + require.NoError(t, err) + + var fakeCtx context.Context + fakeClient := &http.Client{ + Transport: fakeRoundTripper{ + roundTrip: func(req *http.Request) (*http.Response, error) { + if strings.Contains(req.URL.String(), "authorize") { + // This is the user hitting the browser endpoint to begin the OIDC + // auth flow. + + // Authorize should redirect the user back to the app after authentication on + // the IDP. + resp := httptest.NewRecorder() + v := url.Values{ + "code": {oauth2Code}, + "state": {stateString}, + "session_state": {"a18cf797-1e2b-4bc3-baf9-66b41a4997cf"}, + } + + // This url doesn't really matter since the fake client will hiject this actual request. + http.Redirect(resp, req, "http://localhost:3000/api/v2/users/oidc/callback?"+v.Encode(), http.StatusTemporaryRedirect) + return resp.Result(), nil + } + if strings.Contains(req.URL.String(), "v2.0/token") { + vals := assertJWTAuth(t, req) + switch vals.Get("grant_type") { + case "authorization_code": + // Initial token + initialExchange = true + assert.Equal(t, oauth2Code, vals.Get("code"), "initial exchange code mismatch") + case "refresh_token": + // refreshed token + tokenRefreshed = true + assert.Equal(t, "", vals.Get("refresh_token"), "refresh token required") + } + + resp := httptest.NewRecorder() + // Taken from an actual response + // Just always return a token no matter what. + resp.Header().Set("Content-Type", "application/json") + _, _ = resp.Write([]byte(`{ + "token_type":"Bearer", + "scope":"email openid profile AccessReview.ReadWrite.Membership Group.Read.All Group.ReadWrite.All User.Read", + "expires_in":4009, + "ext_expires_in":4009, + "access_token":"", + "refresh_token":"", + "id_token":"eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6Ii1LSTNROW5OUjdiUm9meG1lWm9YcWJIWkdldyJ9.eyJhdWQiOiIxZjAxODMyYS1mZWViLTQyZGMtODFkOS01ZjBhYjZhMDQxZTAiLCJpc3MiOiJodHRwczovL2xvZ2luLm1pY3Jvc29mdG9ubGluZS5jb20vMTEwZjBjMGYtY2Q3Ni00NzE3LWE2ZjgtNGVlYTNkMGY4MTA5L3YyLjAiLCJpYXQiOjE2OTE3OTI2MzQsIm5iZiI6MTY5MTc5MjYzNCwiZXhwIjoxNjkxNzk2NTM0LCJhaW8iOiJBWVFBZS84VUFBQUE1eEtqMmVTdWFXVmZsRlhCeGJJTnMvSkVyVHFvUGlaQW5ENmJIZWF3a2RRcisyRVRwM3RGNGY3akxicnh3ODhhVm9QOThrY0xMNjhON1hVV3FCN1I1N2JQRU9EclRlSUI1S0lyUHBjbCtIeXR0a1ljOVdWQklVVEErSllQbzl1a0ZjbGNWZ1krWUc3eHlmdi90K3Q1ZEczblNuZEdEQ1FYRVIxbDlTNko1T2c9IiwiZW1haWwiOiJzdGV2ZW5AY29kZXIuY29tIiwiZ3JvdXBzIjpbImM4MDQ4ZTkxLWY1YzMtNDdlNS05NjkzLTgzNGRlODQwMzRhZCIsIjcwYjQ4MTc1LTEwN2ItNGFkOC1iNDA1LTRkODg4YTFjNDY2ZiJdLCJpZHAiOiJtYWlsIiwibmFtZSI6IlN0ZXZlbiBNIiwib2lkIjoiN2JhNDYzNjAtZTAyNy00OTVhLTlhZTUtM2FlYWZlMzY3MGEyIiwicHJlZmVycmVkX3VzZXJuYW1lIjoic3RldmVuQGNvZGVyLmNvbSIsInByb3ZfZGF0YSI6W3siQXQiOnRydWUsIlByb3YiOiJnaXRodWIuY29tIiwiQWx0c2VjaWQiOiI1NDQ2Mjk4IiwiQWNjZXNzVG9rZW4iOm51bGx9XSwicmgiOiIwLkFUZ0FEd3dQRVhiTkYwZW0tRTdxUFEtQkNTcURBUl9yX3R4Q2dkbGZDcmFnUWVBNEFPRS4iLCJyb2xlcyI6WyJUZW1wbGF0ZUF1dGhvcnMiXSwic3ViIjoib0JTN3FjUERKdWlDMEYyQ19XdDJycVlvanhpT0o3S3JFWjlkQ1RkTGVYNCIsInRpZCI6IjExMGYwYzBmLWNkNzYtNDcxNy1hNmY4LTRlZWEzZDBmODEwOSIsInV0aSI6IktReGlIWGtaZUVxcC1tQWlVdTlyQUEiLCJ2ZXIiOiIyLjAiLCJyb2xlczIiOiJUZW1wbGF0ZUF1dGhvcnMifQ.JevFI4Xm9dW7kQq4xEgZnUaU0SqbeOAFtT0YIKQNefR9Db4sjxCaKRmX0pPt-CM9j45d6fAiAkLFDAqjlSbi4Zi0GbEomT3yegmuxKgEgjPpJlGjF2TBUpsNNyn5gJ9Wkct9BfwALJhX2ePJFzIlkvx9opNNbNK1qHKMMjOSRFG6AGExKRDiQAME0a4hVgCwrAdUs4JrCcj4LqB84dODN-eoh-jx2-1wDvf6fovfwLHDQwjY4lfBxaYdNavKM369hrhU-U067rSnCzvDD26f4VLhPF52hiQIbTVN5t7p_1XmcduUiaNnmr9AZiZxZ-94mctSRRR8xG0pNwO2yv84iA" + }`)) + return resp.Result(), nil + } + // This is the "Coder" half of things. We can keep this in the fake + // client, essentially being the fake client on both sides of the OIDC + // flow. + if strings.Contains(req.URL.String(), "v2/users/oidc/callback") { + // This is the callback from the IDP. + code := req.URL.Query().Get("code") + require.Equal(t, oauth2Code, code, "code mismatch") + state := req.URL.Query().Get("state") + require.Equal(t, stateString, state, "state mismatch") + + // Exchange for token should work + token, err := pki.Exchange(fakeCtx, code) + if !assert.NoError(t, err) { + return httptest.NewRecorder().Result(), nil + } + + // Also try a refresh + cpy := token + cpy.Expiry = time.Now().Add(time.Minute * -1) + src := pki.TokenSource(fakeCtx, cpy) + _, err = src.Token() + tokenRefreshed = true + assert.NoError(t, err, "token refreshed") + return httptest.NewRecorder().Result(), nil + } + + return nil, xerrors.Errorf("not implemented") + }}, + } + fakeCtx = oidc.ClientContext(context.Background(), fakeClient) + var _ = fakeCtx + + // This simulates a client logging into the browser. The 307 redirect will + // make sure this goes through the full flow. + _, err = fakeClient.Get(pki.AuthCodeURL("state", oauth2.AccessTypeOffline)) + require.NoError(t, err) + + require.True(t, initialExchange, "initial token exchange complete") + require.True(t, tokenRefreshed, "token was refreshed") +} + +type fakeRoundTripper struct { + roundTrip func(req *http.Request) (*http.Response, error) +} + +func (f fakeRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + return f.roundTrip(req) +} + +// assertJWTAuth will assert the basic JWT auth assertions. It will return the +// url.Values from the request body for any additional assertions to be made. +func assertJWTAuth(t *testing.T, r *http.Request) url.Values { + body, err := io.ReadAll(r.Body) + if !assert.NoError(t, err) { + return nil + } + vals, err := url.ParseQuery(string(body)) + if !assert.NoError(t, err) { + return nil + } + + assert.Equal(t, "urn:ietf:params:oauth:client-assertion-type:jwt-bearer", vals.Get("client_assertion_type")) + jwtToken := vals.Get("client_assertion") + // No need to actually verify the jwt is signed right. + parsedToken, _, err := (&jwt.Parser{}).ParseUnverified(jwtToken, jwt.MapClaims{}) + if !assert.NoError(t, err, "failed to parse jwt token") { + return nil + } + + // Azure requirements + assert.NotEmpty(t, parsedToken.Header["x5t"], "hashed cert missing") + assert.Equal(t, "RS256", parsedToken.Header["alg"], "azure only accepts RS256") + assert.Equal(t, "JWT", parsedToken.Header["typ"], "azure only accepts JWT") + + return vals +} From 15db163276aab3a9f0227974f5f1031d1fa4b132 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Aug 2023 09:25:02 -0500 Subject: [PATCH 08/14] Add comments --- cli/server.go | 2 ++ coderd/oauthpki/oidcpki.go | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/cli/server.go b/cli/server.go index 61fbe89e2e049..784db22bb062b 100644 --- a/cli/server.go +++ b/cli/server.go @@ -1521,6 +1521,8 @@ func configureOIDCPKI(orig *oauth2.Config, keyFile string, certFile string) (*oa } var certData []byte + // According to the spec, this is not required. So do not require it on the initial loading + // of the PKI config. if certFile != "" { certData, err = os.ReadFile(certFile) if err != nil { diff --git a/coderd/oauthpki/oidcpki.go b/coderd/oauthpki/oidcpki.go index 1f39ac07eaee3..b57d9bdf3f3ee 100644 --- a/coderd/oauthpki/oidcpki.go +++ b/coderd/oauthpki/oidcpki.go @@ -36,9 +36,13 @@ import ( type Config struct { cfg httpmw.OAuth2Config + // These values should match those provided in the oauth2.Config. + // Because the inner config is an interface, we need to duplicate these + // values here. scopes []string clientID string tokenURL string + // ClientSecret is the private key of the PKI cert. // Azure AD only supports RS256 signing algorithm. clientKey *rsa.PrivateKey From 801799f429acbe2d692a202d8ca9a60127551b5c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Aug 2023 10:00:22 -0500 Subject: [PATCH 09/14] add little comment about constant 5min --- coderd/oauthpki/oidcpki.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/oauthpki/oidcpki.go b/coderd/oauthpki/oidcpki.go index b57d9bdf3f3ee..f1f52ab3c0e64 100644 --- a/coderd/oauthpki/oidcpki.go +++ b/coderd/oauthpki/oidcpki.go @@ -138,6 +138,8 @@ func (ja *Config) jwtToken() (string, error) { "iss": ja.clientID, "sub": ja.clientID, "aud": ja.tokenURL, + // 5-10 minutes is recommended in the Azure docs. + // So we'll use 5 minutes. "exp": now.Add(time.Minute * 5).Unix(), "jti": uuid.New().String(), "nbf": now.Unix(), From 6914619ea8e20ea8b57a75a2f0faf14acd676017 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Aug 2023 13:30:26 -0500 Subject: [PATCH 10/14] Fix typo --- cli/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/server.go b/cli/server.go index 784db22bb062b..30e327710c16d 100644 --- a/cli/server.go +++ b/cli/server.go @@ -554,7 +554,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. if cfg.OIDC.ClientKeyFile != "" || cfg.OIDC.ClientSecret != "" { if cfg.OIDC.ClientID == "" { - return xerrors.Errorf("OIDC client ID be set!") + return xerrors.Errorf("OIDC client ID must be set!") } if cfg.OIDC.IssuerURL == "" { return xerrors.Errorf("OIDC issuer URL must be set!") From cb98e183e85bf54336a0c648f1aaf99641c3657e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Aug 2023 15:04:24 -0500 Subject: [PATCH 11/14] update golden files --- cli/testdata/coder_server_--help.golden | 10 ++++ coderd/apidoc/docs.go | 7 +++ coderd/apidoc/swagger.json | 7 +++ coderd/oauthpki/oidcpki.go | 16 ++++-- coderd/oauthpki/okidcpki_test.go | 12 +++-- docs/api/general.md | 2 + docs/api/schemas.md | 52 +++++++++++-------- docs/cli/server.md | 18 +++++++ .../cli/testdata/coder_server_--help.golden | 10 ++++ 9 files changed, 105 insertions(+), 29 deletions(-) diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index cd8fd309f1154..46fddeed2d6cc 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -304,9 +304,19 @@ can safely ignore these settings. --oidc-auth-url-params struct[map[string]string], $CODER_OIDC_AUTH_URL_PARAMS (default: {"access_type": "offline"}) OIDC auth URL parameters to pass to the upstream provider. + --oidc-client-cert-file string, $CODER_OIDC_CLIENT_CERT_FILE + Pem encoded certificate file to use for oauth2 PKI/JWT authorization. + The public certificate that accompanies oidc-client-key-file. A + standard x509 certificate is expected. + --oidc-client-id string, $CODER_OIDC_CLIENT_ID Client ID to use for Login with OIDC. + --oidc-client-key-file string, $CODER_OIDC_CLIENT_KEY_FILE + Pem encoded RSA private key to use for oauth2 PKI/JWT authorization. + This can be used instead of oidc-client-secret if your IDP supports + it. + --oidc-client-secret string, $CODER_OIDC_CLIENT_SECRET Client secret to use for Login with OIDC. diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 2f04b8be2a3d2..406fe15e422fa 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -8602,9 +8602,16 @@ const docTemplate = `{ "auth_url_params": { "type": "object" }, + "client_cert_file": { + "type": "string" + }, "client_id": { "type": "string" }, + "client_key_file": { + "description": "ClientKeyFile \u0026 ClientCertFile are used in place of ClientSecret for PKI auth.", + "type": "string" + }, "client_secret": { "type": "string" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 997bdca3ade64..c0c71d2888337 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -7715,9 +7715,16 @@ "auth_url_params": { "type": "object" }, + "client_cert_file": { + "type": "string" + }, "client_id": { "type": "string" }, + "client_key_file": { + "description": "ClientKeyFile \u0026 ClientCertFile are used in place of ClientSecret for PKI auth.", + "type": "string" + }, "client_secret": { "type": "string" }, diff --git a/coderd/oauthpki/oidcpki.go b/coderd/oauthpki/oidcpki.go index f1f52ab3c0e64..6e0b00193d22b 100644 --- a/coderd/oauthpki/oidcpki.go +++ b/coderd/oauthpki/oidcpki.go @@ -3,7 +3,7 @@ package oauthpki import ( "context" "crypto/rsa" - "crypto/sha1" + "crypto/sha1" //#nosec // Not used for cryptography. "crypto/x509" "encoding/base64" "encoding/json" @@ -92,6 +92,8 @@ func NewOauth2PKIConfig(params ConfigParams) (*Config, error) { } block, _ := pem.Decode(params.PemEncodedCert) + // Used as an identifier, not an actual cryptographic hash. + //nolint:gosec hashed := sha1.Sum(block.Bytes) return &Config{ @@ -196,7 +198,13 @@ func (src *jwtTokenSource) Token() (*oauth2.Token, error) { "refresh_token": {src.refreshToken}, } // Using params based auth - resp, err := cli.PostForm(src.cfg.tokenURL, v) + req, err := http.NewRequest("POST", src.cfg.tokenURL, strings.NewReader(v.Encode())) + if err != nil { + return nil, xerrors.Errorf("oauth2: make token refresh request: %w", err) + } + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req = req.WithContext(src.ctx) + resp, err := cli.Do(req) if err != nil { return nil, xerrors.Errorf("oauth2: cannot get token: %w", err) } @@ -235,7 +243,7 @@ func (src *jwtTokenSource) Token() (*oauth2.Token, error) { } if unmarshalError != nil { - return nil, fmt.Errorf("oauth2: cannot unmarshal token: %v", err) + return nil, fmt.Errorf("oauth2: cannot unmarshal token: %w", err) } newToken := &oauth2.Token{ @@ -256,7 +264,7 @@ func (src *jwtTokenSource) Token() (*oauth2.Token, error) { // decode returned id token to get expiry claimSet, err := jws.Decode(v) if err != nil { - return nil, fmt.Errorf("oauth2: error decoding JWT token: %v", err) + return nil, fmt.Errorf("oauth2: error decoding JWT token: %w", err) } newToken.Expiry = time.Unix(claimSet.Exp, 0) } diff --git a/coderd/oauthpki/okidcpki_test.go b/coderd/oauthpki/okidcpki_test.go index d7d45a27dad3f..6f4f31afbc02c 100644 --- a/coderd/oauthpki/okidcpki_test.go +++ b/coderd/oauthpki/okidcpki_test.go @@ -84,6 +84,8 @@ B1B7CpkMU55hPP+7nsofCszNrMDXT8Z5w2a3zLKM // It runs an oauth2.Exchange method and hijacks the request to only check the // request side of the transaction. func TestAzureADPKIOIDC(t *testing.T) { + t.Parallel() + oauthCfg := &oauth2.Config{ ClientID: "random-client-id", Endpoint: oauth2.Endpoint{ @@ -128,6 +130,8 @@ func TestAzureADPKIOIDC(t *testing.T) { // to prevent some regressions by running a full "e2e" oauth and asserting some // of the request values. func TestSavedAzureADPKIOIDC(t *testing.T) { + t.Parallel() + var ( stateString = "random-state" oauth2Code = base64.StdEncoding.EncodeToString([]byte("random-code")) @@ -237,15 +241,17 @@ func TestSavedAzureADPKIOIDC(t *testing.T) { } return nil, xerrors.Errorf("not implemented") - }}, + }, + }, } fakeCtx = oidc.ClientContext(context.Background(), fakeClient) - var _ = fakeCtx + _ = fakeCtx // This simulates a client logging into the browser. The 307 redirect will // make sure this goes through the full flow. - _, err = fakeClient.Get(pki.AuthCodeURL("state", oauth2.AccessTypeOffline)) + resp, err := fakeClient.Get(pki.AuthCodeURL("state", oauth2.AccessTypeOffline)) require.NoError(t, err) + _ = resp.Body.Close() require.True(t, initialExchange, "initial token exchange complete") require.True(t, tokenRefreshed, "token was refreshed") diff --git a/docs/api/general.md b/docs/api/general.md index 62341f1dcbf27..170d52a0f3260 100644 --- a/docs/api/general.md +++ b/docs/api/general.md @@ -256,7 +256,9 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "oidc": { "allow_signups": true, "auth_url_params": {}, + "client_cert_file": "string", "client_id": "string", + "client_key_file": "string", "client_secret": "string", "email_domain": ["string"], "email_field": "string", diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 11aad4ba5f814..f40e8e65486f0 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -2073,7 +2073,9 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "oidc": { "allow_signups": true, "auth_url_params": {}, + "client_cert_file": "string", "client_id": "string", + "client_key_file": "string", "client_secret": "string", "email_domain": ["string"], "email_field": "string", @@ -2433,7 +2435,9 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "oidc": { "allow_signups": true, "auth_url_params": {}, + "client_cert_file": "string", "client_id": "string", + "client_key_file": "string", "client_secret": "string", "email_domain": ["string"], "email_field": "string", @@ -3346,7 +3350,9 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in { "allow_signups": true, "auth_url_params": {}, + "client_cert_file": "string", "client_id": "string", + "client_key_file": "string", "client_secret": "string", "email_domain": ["string"], "email_field": "string", @@ -3381,28 +3387,30 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in ### Properties -| Name | Type | Required | Restrictions | Description | -| ----------------------- | -------------------------------- | -------- | ------------ | ----------- | -| `allow_signups` | boolean | false | | | -| `auth_url_params` | object | false | | | -| `client_id` | string | false | | | -| `client_secret` | string | false | | | -| `email_domain` | array of string | false | | | -| `email_field` | string | false | | | -| `group_auto_create` | boolean | false | | | -| `group_mapping` | object | false | | | -| `group_regex_filter` | [clibase.Regexp](#clibaseregexp) | false | | | -| `groups_field` | string | false | | | -| `icon_url` | [clibase.URL](#clibaseurl) | false | | | -| `ignore_email_verified` | boolean | false | | | -| `ignore_user_info` | boolean | false | | | -| `issuer_url` | string | false | | | -| `scopes` | array of string | false | | | -| `sign_in_text` | string | false | | | -| `user_role_field` | string | false | | | -| `user_role_mapping` | object | false | | | -| `user_roles_default` | array of string | false | | | -| `username_field` | string | false | | | +| Name | Type | Required | Restrictions | Description | +| ----------------------- | -------------------------------- | -------- | ------------ | -------------------------------------------------------------------------------- | +| `allow_signups` | boolean | false | | | +| `auth_url_params` | object | false | | | +| `client_cert_file` | string | false | | | +| `client_id` | string | false | | | +| `client_key_file` | string | false | | Client key file & ClientCertFile are used in place of ClientSecret for PKI auth. | +| `client_secret` | string | false | | | +| `email_domain` | array of string | false | | | +| `email_field` | string | false | | | +| `group_auto_create` | boolean | false | | | +| `group_mapping` | object | false | | | +| `group_regex_filter` | [clibase.Regexp](#clibaseregexp) | false | | | +| `groups_field` | string | false | | | +| `icon_url` | [clibase.URL](#clibaseurl) | false | | | +| `ignore_email_verified` | boolean | false | | | +| `ignore_user_info` | boolean | false | | | +| `issuer_url` | string | false | | | +| `scopes` | array of string | false | | | +| `sign_in_text` | string | false | | | +| `user_role_field` | string | false | | | +| `user_role_mapping` | object | false | | | +| `user_roles_default` | array of string | false | | | +| `username_field` | string | false | | | ## codersdk.Organization diff --git a/docs/cli/server.md b/docs/cli/server.md index 9c7a90893ac9e..cda80beb7dbf8 100644 --- a/docs/cli/server.md +++ b/docs/cli/server.md @@ -418,6 +418,15 @@ Whether new users can sign up with OIDC. OIDC auth URL parameters to pass to the upstream provider. +### --oidc-client-cert-file + +| | | +| ----------- | ----------------------------------------- | +| Type | string | +| Environment | $CODER_OIDC_CLIENT_CERT_FILE | + +Pem encoded certificate file to use for oauth2 PKI/JWT authorization. The public certificate that accompanies oidc-client-key-file. A standard x509 certificate is expected. + ### --oidc-client-id | | | @@ -428,6 +437,15 @@ OIDC auth URL parameters to pass to the upstream provider. Client ID to use for Login with OIDC. +### --oidc-client-key-file + +| | | +| ----------- | ---------------------------------------- | +| Type | string | +| Environment | $CODER_OIDC_CLIENT_KEY_FILE | + +Pem encoded RSA private key to use for oauth2 PKI/JWT authorization. This can be used instead of oidc-client-secret if your IDP supports it. + ### --oidc-client-secret | | | diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index cd8fd309f1154..46fddeed2d6cc 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/enterprise/cli/testdata/coder_server_--help.golden @@ -304,9 +304,19 @@ can safely ignore these settings. --oidc-auth-url-params struct[map[string]string], $CODER_OIDC_AUTH_URL_PARAMS (default: {"access_type": "offline"}) OIDC auth URL parameters to pass to the upstream provider. + --oidc-client-cert-file string, $CODER_OIDC_CLIENT_CERT_FILE + Pem encoded certificate file to use for oauth2 PKI/JWT authorization. + The public certificate that accompanies oidc-client-key-file. A + standard x509 certificate is expected. + --oidc-client-id string, $CODER_OIDC_CLIENT_ID Client ID to use for Login with OIDC. + --oidc-client-key-file string, $CODER_OIDC_CLIENT_KEY_FILE + Pem encoded RSA private key to use for oauth2 PKI/JWT authorization. + This can be used instead of oidc-client-secret if your IDP supports + it. + --oidc-client-secret string, $CODER_OIDC_CLIENT_SECRET Client secret to use for Login with OIDC. From 14796449a04b7cbe5604140de55dfc4f8e27bc02 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Aug 2023 15:21:03 -0500 Subject: [PATCH 12/14] Test linting --- coderd/oauthpki/okidcpki_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/oauthpki/okidcpki_test.go b/coderd/oauthpki/okidcpki_test.go index 6f4f31afbc02c..fe1609c8c4c34 100644 --- a/coderd/oauthpki/okidcpki_test.go +++ b/coderd/oauthpki/okidcpki_test.go @@ -249,6 +249,7 @@ func TestSavedAzureADPKIOIDC(t *testing.T) { // This simulates a client logging into the browser. The 307 redirect will // make sure this goes through the full flow. + // nolint: noctx resp, err := fakeClient.Get(pki.AuthCodeURL("state", oauth2.AccessTypeOffline)) require.NoError(t, err) _ = resp.Body.Close() From 0f508f542b67e77514e5f5e6fbdc3379753e839f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Aug 2023 15:59:21 -0500 Subject: [PATCH 13/14] Add yaml config options --- coderd/oauthpki/okidcpki_test.go | 1 + codersdk/deployment.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/coderd/oauthpki/okidcpki_test.go b/coderd/oauthpki/okidcpki_test.go index fe1609c8c4c34..52f1223bcab50 100644 --- a/coderd/oauthpki/okidcpki_test.go +++ b/coderd/oauthpki/okidcpki_test.go @@ -99,6 +99,7 @@ func TestAzureADPKIOIDC(t *testing.T) { PemEncodedKey: []byte(testClientKey), PemEncodedCert: []byte(testClientCert), Config: oauthCfg, + Scopes: []string{"openid", "email", "profile"}, }) require.NoError(t, err, "failed to create pki config") diff --git a/codersdk/deployment.go b/codersdk/deployment.go index b1a1223e27153..1563a6eb1225d 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -977,6 +977,7 @@ when required by your organization's security policy.`, "This can be used instead of oidc-client-secret if your IDP supports it.", Flag: "oidc-client-key-file", Env: "CODER_OIDC_CLIENT_KEY_FILE", + YAML: "oidcClientKeyFile", Value: &c.OIDC.ClientKeyFile, Group: &deploymentGroupOIDC, }, @@ -986,6 +987,7 @@ when required by your organization's security policy.`, "The public certificate that accompanies oidc-client-key-file. A standard x509 certificate is expected.", Flag: "oidc-client-cert-file", Env: "CODER_OIDC_CLIENT_CERT_FILE", + YAML: "oidcClientCertFile", Value: &c.OIDC.ClientCertFile, Group: &deploymentGroupOIDC, }, From 189007f4f02b1f850e5d90b226639f06df56f301 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Aug 2023 17:07:05 -0500 Subject: [PATCH 14/14] make gen again... --- cli/testdata/server-config.yaml.golden | 9 +++++++++ docs/cli/server.md | 2 ++ 2 files changed, 11 insertions(+) diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index 27aeb41a61b6a..381920662cf05 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -244,6 +244,15 @@ oidc: # Client ID to use for Login with OIDC. # (default: , type: string) clientID: "" + # Pem encoded RSA private key to use for oauth2 PKI/JWT authorization. This can be + # used instead of oidc-client-secret if your IDP supports it. + # (default: , type: string) + oidcClientKeyFile: "" + # Pem encoded certificate file to use for oauth2 PKI/JWT authorization. The public + # certificate that accompanies oidc-client-key-file. A standard x509 certificate + # is expected. + # (default: , type: string) + oidcClientCertFile: "" # Email domains that clients logging in with OIDC must match. # (default: , type: string-array) emailDomain: [] diff --git a/docs/cli/server.md b/docs/cli/server.md index cda80beb7dbf8..a2f29c39dca04 100644 --- a/docs/cli/server.md +++ b/docs/cli/server.md @@ -424,6 +424,7 @@ OIDC auth URL parameters to pass to the upstream provider. | ----------- | ----------------------------------------- | | Type | string | | Environment | $CODER_OIDC_CLIENT_CERT_FILE | +| YAML | oidc.oidcClientCertFile | Pem encoded certificate file to use for oauth2 PKI/JWT authorization. The public certificate that accompanies oidc-client-key-file. A standard x509 certificate is expected. @@ -443,6 +444,7 @@ Client ID to use for Login with OIDC. | ----------- | ---------------------------------------- | | Type | string | | Environment | $CODER_OIDC_CLIENT_KEY_FILE | +| YAML | oidc.oidcClientKeyFile | Pem encoded RSA private key to use for oauth2 PKI/JWT authorization. This can be used instead of oidc-client-secret if your IDP supports it.