Skip to content

Commit

Permalink
revert OIDC IdP CA to use RSA keys in new suites
Browse files Browse the repository at this point in the history
I never actually updated any of our OIDC IdP code to support ECDSA, it
actually just doesn't support it at all.

It also goes against the OIDC spec to not support the RS256 algorithm for
id_token_signing_alg_values_supported, which we wouldn't be able to
support if we didn't have an RSA key.

I've already reverted the SAML IdP as well. We'll need a different
strategy to offer both RSA and ECDSA in the future.
  • Loading branch information
nklaassen committed Sep 20, 2024
1 parent 5e921b3 commit dccda3b
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 54 deletions.
4 changes: 2 additions & 2 deletions lib/client/ca_export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func TestExportAuthorities(t *testing.T) {
require.NotNil(t, key.Signer, "ParsePrivateKey returned a nil key")
}

// TestAuthServer uses ECDSA for all CAs except db_client.
// TestAuthServer uses ECDSA for all CAs except db, db_client, saml_idp, oidc_idp.
validateRSAPrivateKeyDERFunc := func(t *testing.T, s string) {
privKey, err := x509.ParsePKCS1PrivateKey([]byte(s))
require.NoError(t, err, "x509.ParsePKCS1PrivateKey failed")
Expand Down Expand Up @@ -227,7 +227,7 @@ func TestExportAuthorities(t *testing.T) {
},
errorCheck: require.NoError,
assertNoSecrets: validateTLSCertificateDERFunc,
assertSecrets: validateECDSAPrivateKeyDERFunc,
assertSecrets: validateRSAPrivateKeyDERFunc,
},
{
name: "db-client",
Expand Down
6 changes: 3 additions & 3 deletions lib/cryptosuites/suites.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ var (
DatabaseClientCATLS: RSA2048,
OpenSSHCASSH: Ed25519,
JWTCAJWT: ECDSAP256,
OIDCIdPCAJWT: ECDSAP256,
OIDCIdPCAJWT: RSA2048,
SAMLIdPCATLS: RSA2048,
SPIFFECATLS: ECDSAP256,
SPIFFECAJWT: ECDSAP256,
Expand Down Expand Up @@ -214,7 +214,7 @@ var (
DatabaseClientCATLS: RSA2048,
OpenSSHCASSH: ECDSAP256,
JWTCAJWT: ECDSAP256,
OIDCIdPCAJWT: ECDSAP256,
OIDCIdPCAJWT: RSA2048,
SAMLIdPCATLS: RSA2048,
SPIFFECATLS: ECDSAP256,
SPIFFECAJWT: ECDSAP256,
Expand Down Expand Up @@ -244,7 +244,7 @@ var (
DatabaseClientCATLS: RSA2048,
OpenSSHCASSH: ECDSAP256,
JWTCAJWT: ECDSAP256,
OIDCIdPCAJWT: ECDSAP256,
OIDCIdPCAJWT: RSA2048,
SAMLIdPCATLS: RSA2048,
SPIFFECATLS: ECDSAP256,
SPIFFECAJWT: ECDSAP256,
Expand Down
23 changes: 12 additions & 11 deletions lib/services/suite/suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,30 +83,31 @@ func NewTestCAWithConfig(config TestCAConfig) *types.CertAuthorityV2 {
var keyPEM []byte
var key *keys.PrivateKey

if config.Type == types.DatabaseClientCA {
// Always use pre-generated RSA key for the db_client CA.
keyPEM = fixtures.PEMBytes["rsa-db-client"]
}
if config.Type == types.SAMLIDPCA {
// The SAML IdP uses xmldsig RSA SHA256 signature method
// http://www.w3.org/2001/04/xmldsig-more#rsa-sha256
// to sign the SAML assertion, so the key must be an RSA key.
switch config.Type {
case types.DatabaseCA, types.SAMLIDPCA, types.OIDCIdPCA:
// These CAs only support RSA.
keyPEM = fixtures.PEMBytes["rsa"]
case types.DatabaseClientCA:
// The db client CA also only supports RSA, but some tests rely on it
// being different than the DB CA.
keyPEM = fixtures.PEMBytes["rsa-db-client"]
}
if len(config.PrivateKeys) > 0 {
// Allow test to override the private key.
keyPEM = config.PrivateKeys[0]
}

if keyPEM != nil {
var err error
key, err = keys.ParsePrivateKey(keyPEM)
if err != nil {
panic(err)
}
} else {
// If config.PrivateKeys was not set and this is not the db_client CA,
// generate an ECDSA key. Signatures are ~10x faster than RSA and
// generating a new key is actually faster than parsing a PEM fixture.
// If config.PrivateKeys was not set and this CA does not exclusively
// support RSA, generate an ECDSA key. Signatures are ~10x faster than
// RSA and generating a new key is actually faster than parsing a PEM
// fixture.
signer, err := cryptosuites.GenerateKeyWithAlgorithm(cryptosuites.ECDSAP256)
if err != nil {
panic(err)
Expand Down
98 changes: 60 additions & 38 deletions lib/web/oidcidp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/gravitational/teleport/lib"
Expand All @@ -43,48 +42,71 @@ func TestOIDCIdPPublicEndpoints(t *testing.T) {
resp, err := publicClt.Get(ctx, proxy.webURL.String()+"/.well-known/openid-configuration", nil)
require.NoError(t, err)

jwksURI := struct {
JWKSURI string `json:"jwks_uri"`
Issuer string `json:"issuer"`
}{}

err = json.Unmarshal(resp.Bytes(), &jwksURI)
require.NoError(t, err)

// Proxy Public addr must match with Issuer
require.Equal(t, proxy.webURL.String(), jwksURI.Issuer)

// Follow the `jwks_uri` endpoint and fetch the public keys
require.NotEmpty(t, jwksURI.JWKSURI)
resp, err = publicClt.Get(ctx, jwksURI.JWKSURI, nil)
// Deliberately redefining the structs in this test to assert that the JSON
// representation doesn't unintentionally change.
type oidcConfiguration struct {
Issuer string `json:"issuer"`
JWKSURI string `json:"jwks_uri"`
Claims []string `json:"claims"`
IdTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"`
ResponseTypesSupported []string `json:"response_types_supported"`
ScopesSupported []string `json:"scopes_supported"`
SubjectTypesSupported []string `json:"subject_types_supported"`
}

var gotConfiguration oidcConfiguration
require.NoError(t, json.Unmarshal(resp.Bytes(), &gotConfiguration))

expectedConfiguration := oidcConfiguration{
Issuer: proxy.webURL.String(),
JWKSURI: proxy.webURL.String() + "/.well-known/jwks-oidc",
// OIDC IdPs MUST support RSA256 here.
IdTokenSigningAlgValuesSupported: []string{"RS256"},
Claims: []string{"iss", "sub", "obo", "aud", "jti", "iat", "exp", "nbf"},
ResponseTypesSupported: []string{"id_token"},
ScopesSupported: []string{"openid"},
SubjectTypesSupported: []string{"public", "pair-wise"},
}
require.Equal(t, expectedConfiguration, gotConfiguration)

resp, err = publicClt.Get(ctx, gotConfiguration.JWKSURI, nil)
require.NoError(t, err)

jwksKeys := struct {
Keys []struct {
Use string `json:"use"`
KeyID *string `json:"kid"`
KeyType string `json:"kty"`
Alg string `json:"alg"`
} `json:"keys"`
}{}

err = json.Unmarshal(resp.Bytes(), &jwksKeys)
type jwksKey struct {
Use string `json:"use"`
KeyID *string `json:"kid"`
KeyType string `json:"kty"`
Alg string `json:"alg"`
}
type jwksKeys struct {
Keys []jwksKey `json:"keys"`
}

var gotKeys jwksKeys
err = json.Unmarshal(resp.Bytes(), &gotKeys)
require.NoError(t, err)

require.NotEmpty(t, jwksKeys.Keys)
require.Len(t, jwksKeys.Keys, 2)

// Expect the same key twice, once with a synthesized Key ID, and once with an empty Key ID for compatibility.
key1 := jwksKeys.Keys[0]
key2 := jwksKeys.Keys[1]
assert.Equal(t, "sig", key1.Use)
assert.Equal(t, "EC", key1.KeyType)
assert.Equal(t, "ES256", key1.Alg)
assert.Equal(t, key1.Use, key2.Use)
assert.Equal(t, key1.KeyType, key2.KeyType)
assert.Equal(t, key1.Alg, key2.Alg)
assert.NotEmpty(t, *key1.KeyID)
assert.Equal(t, "", *key2.KeyID)
require.Len(t, gotKeys.Keys, 2)
require.NotEmpty(t, *gotKeys.Keys[0].KeyID)
require.Equal(t, "", *gotKeys.Keys[1].KeyID)
expectedKeys := jwksKeys{
Keys: []jwksKey{
{
Use: "sig",
KeyType: "RSA",
Alg: "RS256",
KeyID: gotKeys.Keys[0].KeyID,
},
{
Use: "sig",
KeyType: "RSA",
Alg: "RS256",
KeyID: new(string),
},
},
}
require.Equal(t, expectedKeys, gotKeys)
}

func TestThumbprint(t *testing.T) {
Expand Down

0 comments on commit dccda3b

Please sign in to comment.