From d68efb6df990d484b33139a11c02a53985292151 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Wed, 9 Sep 2026 18:31:12 +0400 Subject: [PATCH] fix: restore IPA KRA compatibility and fix JWK parse panic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `293d9f50` migrated `auth/iam_ipa.go`'s KRA session-key wrap from `rsa.EncryptPKCS1v15` to `rsa.EncryptOAEP` to silence a Go 1.26 deprecation warning. FreeIPA's KRA can be configured to unwrap session keys with either PKCS#1 v1.5 (its default) or OAEP, and the REST API has no way to query which one a given deployment uses — the same constraint FreeIPA's own client (`ipaclient/plugins/vault.py`) works around by trying one padding and falling back to the other. Hardcoding OAEP with no fallback breaks `GetUserAccount` against any KRA using the default PKCS#1 v1.5 configuration, which includes every deployment that worked before that change. This restores compatibility by trying PKCS#1 v1.5 first, matching the pre-`293d9f50` behavior, and falling back to OAEP on failure so FIPS-mode KRAs (which reject PKCS#1 v1.5) keep working too. Separately, `293d9f50` also changed `iamapi/internal/iamutil/webidentity.go`'s OIDC JWKS parsing to build EC public keys via `ecdsa.ParseUncompressedPublicKey` instead of setting `ecdsa.PublicKey`'s `X`/`Y` fields directly — a real improvement, since it validates the point is on the curve, which the old code never did. But it writes the JWK's `x`/`y` coordinates into a fixed-size buffer via `big.Int.FillBytes` without checking their length first, so an oversized `x` or `y` in a JWKS response panics instead of returning an error. That JWKS is fetched from the OIDC issuer configured on a role's trust policy, so a malformed or compromised response can crash request handling for `AssumeRoleWithWebIdentity`. This adds a bounds check before the `FillBytes` calls, plus `TestJwkPublicKeyEC` covering both the valid round-trip and the oversized-coordinate case, since `jwk.publicKey()`'s EC branch had no prior test coverage. --- auth/iam_ipa.go | 72 +++++++++++++++------ iamapi/internal/iamutil/webidentity.go | 10 ++- iamapi/internal/iamutil/webidentity_test.go | 45 +++++++++++++ 3 files changed, 104 insertions(+), 23 deletions(-) diff --git a/auth/iam_ipa.go b/auth/iam_ipa.go index 490d4948..a490077a 100644 --- a/auth/iam_ipa.go +++ b/auth/iam_ipa.go @@ -169,27 +169,19 @@ func (ipa *IpaIAMService) GetUserAccount(access string) (Account, error) { return account, fmt.Errorf("ipa cannot generate session key: %w", err) } - encryptedKey, err := rsa.EncryptOAEP(sha256.New(), rand.Reader, ipa.kraTransportKey, session_key, nil) + // FreeIPA's KRA can be configured to unwrap session keys with either + // RSA PKCS#1 v1.5 (its default) or RSA-OAEP (used under FIPS mode), and + // the REST API gives no way to tell which one a given deployment uses. + // FreeIPA's own client hits the same wall - see the use_oaep fallback in + // ipaclient/plugins/vault.py's _do_internal - so try the default first + // and fall back to OAEP so both KRA configurations keep working. + data, err := ipa.retrieveVaultSecret(access, session_key, false) if err != nil { - return account, fmt.Errorf("ipa vault secret retrieval: %w", err) - } - - req, err = ipa.newRequest("vault_retrieve_internal/1", []string{ipa.vaultName}, - map[string]any{"username": access, - "session_key": Base64EncodedWrapped(encryptedKey), - "wrapping_algo": "aes-128-cbc"}) - if err != nil { - return Account{}, fmt.Errorf("ipa vault_retrieve_internal: %w", err) - } - - data := struct { - Vault_data Base64EncodedWrapped - Nonce Base64EncodedWrapped - }{} - - err = ipa.rpc(req, &data) - if err != nil { - return account, err + debuglogger.IAMLogf("ipa vault_retrieve_internal with PKCS1v15 session key wrap failed, retrying with OAEP: %v", err) + data, err = ipa.retrieveVaultSecret(access, session_key, true) + if err != nil { + return account, err + } } aes, err := aes.NewCipher(session_key) @@ -212,6 +204,46 @@ func (ipa *IpaIAMService) GetUserAccount(access string) (Account, error) { return account, nil } +type vaultSecretData struct { + Vault_data Base64EncodedWrapped + Nonce Base64EncodedWrapped +} + +// retrieveVaultSecret RSA-wraps sessionKey with the KRA transport key and +// calls vault_retrieve_internal to fetch the account secret it protects. +// useOAEP selects RSA-OAEP wrapping instead of the PKCS#1 v1.5 default. +func (ipa *IpaIAMService) retrieveVaultSecret(access string, sessionKey []byte, useOAEP bool) (vaultSecretData, error) { + var ( + encryptedKey []byte + err error + ) + if useOAEP { + encryptedKey, err = rsa.EncryptOAEP(sha256.New(), rand.Reader, ipa.kraTransportKey, sessionKey, nil) + } else { + //lint:ignore SA1019 Reason: PKCS#1 v1.5 is the FreeIPA KRA's default + //session key wrapping scheme and required for protocol compatibility + //with it; see the fallback this feeds in GetUserAccount. + encryptedKey, err = rsa.EncryptPKCS1v15(rand.Reader, ipa.kraTransportKey, sessionKey) + } + if err != nil { + return vaultSecretData{}, fmt.Errorf("ipa vault secret retrieval: %w", err) + } + + req, err := ipa.newRequest("vault_retrieve_internal/1", []string{ipa.vaultName}, + map[string]any{"username": access, + "session_key": Base64EncodedWrapped(encryptedKey), + "wrapping_algo": "aes-128-cbc"}) + if err != nil { + return vaultSecretData{}, fmt.Errorf("ipa vault_retrieve_internal: %w", err) + } + + var data vaultSecretData + if err := ipa.rpc(req, &data); err != nil { + return vaultSecretData{}, err + } + return data, nil +} + // ResolveAccounts returns the subset of accessKeyIDs that do not exist. func (ipa *IpaIAMService) ResolveAccounts(accessKeyIDs []string) ([]string, error) { return resolveAccountsByLookup(accessKeyIDs, ipa.GetUserAccount) diff --git a/iamapi/internal/iamutil/webidentity.go b/iamapi/internal/iamutil/webidentity.go index 4a27761c..721d8e6e 100644 --- a/iamapi/internal/iamutil/webidentity.go +++ b/iamapi/internal/iamutil/webidentity.go @@ -543,12 +543,16 @@ func (k jwk) publicKey() (any, error) { if err != nil { return nil, fmt.Errorf("decode EC y: %w", err) } + byteLen := (curve.Params().BitSize + 7) / 8 + if len(xb) > byteLen || len(yb) > byteLen { + return nil, fmt.Errorf("EC coordinate too large for curve %q", k.Crv) + } x := new(big.Int).SetBytes(xb) y := new(big.Int).SetBytes(yb) - keyBytes := make([]byte, 1+(curve.Params().BitSize+7)/8*2) + keyBytes := make([]byte, 1+byteLen*2) keyBytes[0] = 0x04 - xBytes := x.FillBytes(make([]byte, (curve.Params().BitSize+7)/8)) - yBytes := y.FillBytes(make([]byte, (curve.Params().BitSize+7)/8)) + xBytes := x.FillBytes(make([]byte, byteLen)) + yBytes := y.FillBytes(make([]byte, byteLen)) copy(keyBytes[1:1+len(xBytes)], xBytes) copy(keyBytes[1+len(xBytes):], yBytes) return ecdsa.ParseUncompressedPublicKey(curve, keyBytes) diff --git a/iamapi/internal/iamutil/webidentity_test.go b/iamapi/internal/iamutil/webidentity_test.go index de4d8547..4147222d 100644 --- a/iamapi/internal/iamutil/webidentity_test.go +++ b/iamapi/internal/iamutil/webidentity_test.go @@ -16,6 +16,8 @@ package iamutil import ( "context" + "crypto/ecdsa" + "crypto/elliptic" "crypto/rand" "crypto/rsa" "crypto/tls" @@ -627,3 +629,46 @@ func TestForceRefreshJWKSCacheGatesFailedAttempts(t *testing.T) { t.Errorf("forceRefreshJWKSCache re-attempted a fetch within jwksMinForcedRefreshInterval: lastForcedRefresh changed from %v to %v", before, after) } } + +func TestJwkPublicKeyEC(t *testing.T) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("generate EC key: %v", err) + } + byteLen := (priv.Curve.Params().BitSize + 7) / 8 + // Uncompressed point: 0x04 || X || Y. Derived via PublicKey.Bytes + // rather than the deprecated X/Y fields directly. + pointBytes, err := priv.PublicKey.Bytes() + if err != nil { + t.Fatalf("encode EC public key: %v", err) + } + validX := base64.RawURLEncoding.EncodeToString(pointBytes[1 : 1+byteLen]) + validY := base64.RawURLEncoding.EncodeToString(pointBytes[1+byteLen:]) + + t.Run("valid coordinates round-trip to the same key", func(t *testing.T) { + k := jwk{Kty: "EC", Crv: "P-256", X: validX, Y: validY} + pub, err := k.publicKey() + if err != nil { + t.Fatalf("publicKey() error = %v", err) + } + ecPub, ok := pub.(*ecdsa.PublicKey) + if !ok { + t.Fatalf("publicKey() returned %T, want *ecdsa.PublicKey", pub) + } + if !ecPub.Equal(&priv.PublicKey) { + t.Fatalf("publicKey() returned a key that doesn't match the source key") + } + }) + + // Regression test: an oversized "x"/"y" JWK field (no length check + // existed before) used to panic in big.Int.FillBytes instead of + // returning an error, which took down the request instead of failing + // cleanly with an "invalid identity token" style error. + t.Run("oversized coordinate errors instead of panicking", func(t *testing.T) { + oversized := base64.RawURLEncoding.EncodeToString(make([]byte, byteLen+1)) + k := jwk{Kty: "EC", Crv: "P-256", X: oversized, Y: validY} + if _, err := k.publicKey(); err == nil { + t.Fatal("publicKey() error = nil, want an error for an oversized x coordinate") + } + }) +}