Skip to content

Commit a663927

Browse files
aarmamalnraeneasr
authored
fix: ensure RSA key length fullfills 4096bit requirement (#2905) (#3402)
Closes #2905 Co-authored-by: Arne <[email protected]> Co-authored-by: aeneasr <[email protected]>
1 parent af40d16 commit a663927

File tree

3 files changed

+75
-49
lines changed

3 files changed

+75
-49
lines changed

‎hsm/manager_hsm.go‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ type KeyManager struct {
4242
jwk.Manager
4343
sync.RWMutex
4444
Context
45-
KeySetPrefix string
45+
c config.DefaultProvider
4646
}
4747

4848
var ErrPreGeneratedKeys = &fosite.RFC6749Error{
@@ -53,8 +53,8 @@ var ErrPreGeneratedKeys = &fosite.RFC6749Error{
5353

5454
func NewKeyManager(hsm Context, config *config.DefaultProvider) *KeyManager {
5555
return &KeyManager{
56-
Context: hsm,
57-
KeySetPrefix: config.HSMKeySetPrefix(),
56+
Context: hsm,
57+
c: *config,
5858
}
5959
}
6060

@@ -142,7 +142,7 @@ func (m *KeyManager) GetKey(ctx context.Context, set, kid string) (*jose.JSONWeb
142142
return nil, errors.WithStack(x.ErrNotFound)
143143
}
144144

145-
id, alg, use, err := getKeySetAttributes(m, keyPair, []byte(kid))
145+
id, alg, use, err := m.getKeySetAttributes(ctx, keyPair, []byte(kid))
146146
if err != nil {
147147
return nil, err
148148
}
@@ -174,7 +174,7 @@ func (m *KeyManager) GetKeySet(ctx context.Context, set string) (*jose.JSONWebKe
174174

175175
var keys []jose.JSONWebKey
176176
for _, keyPair := range keyPairs {
177-
kid, alg, use, err := getKeySetAttributes(m, keyPair, nil)
177+
kid, alg, use, err := m.getKeySetAttributes(ctx, keyPair, nil)
178178
if err != nil {
179179
return nil, err
180180
}
@@ -263,7 +263,7 @@ func (m *KeyManager) UpdateKeySet(_ context.Context, _ string, _ *jose.JSONWebKe
263263
return errors.WithStack(ErrPreGeneratedKeys)
264264
}
265265

266-
func getKeySetAttributes(m *KeyManager, key crypto11.Signer, kid []byte) (string, string, string, error) {
266+
func (m *KeyManager) getKeySetAttributes(ctx context.Context, key crypto11.Signer, kid []byte) (string, string, string, error) {
267267
if kid == nil {
268268
ckaId, err := m.GetAttribute(key, crypto11.CkaId)
269269
if err != nil {
@@ -276,8 +276,9 @@ func getKeySetAttributes(m *KeyManager, key crypto11.Signer, kid []byte) (string
276276
switch k := key.Public().(type) {
277277
case *rsa.PublicKey:
278278
alg = "RS256"
279-
// TODO Should we validate minimal key length by checking CKA_MODULUS_BITS?
280-
// TODO see https://github.com/ory/hydra/issues/2905
279+
if k.N.BitLen() < 4096 && !m.c.IsDevelopmentMode(ctx) {
280+
return "", "", "", errors.WithStack(jwk.ErrMinimalRsaKeyLength)
281+
}
281282
case *ecdsa.PublicKey:
282283
if k.Curve == elliptic.P521() {
283284
alg = "ES512"
@@ -365,5 +366,5 @@ func createKeys(key crypto11.Signer, kid, alg, use string) []jose.JSONWebKey {
365366
}
366367

367368
func (m *KeyManager) prefixKeySet(set string) string {
368-
return fmt.Sprintf("%s%s", m.KeySetPrefix, set)
369+
return fmt.Sprintf("%s%s", m.c.HSMKeySetPrefix(), set)
369370
}

‎hsm/manager_hsm_test.go‎

Lines changed: 59 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -62,78 +62,97 @@ func TestKeyManager_HsmKeySetPrefix(t *testing.T) {
6262
ctrl := gomock.NewController(t)
6363
hsmContext := NewMockContext(ctrl)
6464
defer ctrl.Finish()
65+
l := logrusx.New("", "")
66+
c := config.MustNew(context.Background(), l, configx.SkipValidation())
67+
keySetPrefix := "application_specific_prefix."
68+
c.MustSet(context.Background(), config.HSMKeySetPrefix, keySetPrefix)
69+
m := hsm.NewKeyManager(hsmContext, c)
6570

66-
rsaKey, err := rsa.GenerateKey(rand.Reader, 512)
71+
rsaKey3072, err := rsa.GenerateKey(rand.Reader, 3072)
72+
require.NoError(t, err)
73+
rsaKey4096, err := rsa.GenerateKey(rand.Reader, 4096)
6774
require.NoError(t, err)
6875

6976
ecdsaKey, err := ecdsa.GenerateKey(elliptic.P521(), rand.Reader)
7077
require.NoError(t, err)
7178

72-
rsaKeyPair := NewMockSignerDecrypter(ctrl)
73-
rsaKeyPair.EXPECT().Public().Return(&rsaKey.PublicKey).AnyTimes()
79+
rsaKeyPair3072 := NewMockSignerDecrypter(ctrl)
80+
rsaKeyPair3072.EXPECT().Public().Return(&rsaKey3072.PublicKey).AnyTimes()
81+
82+
rsaKeyPair4096 := NewMockSignerDecrypter(ctrl)
83+
rsaKeyPair4096.EXPECT().Public().Return(&rsaKey4096.PublicKey).AnyTimes()
7484

7585
ecdsaKeyPair := NewMockSignerDecrypter(ctrl)
7686
ecdsaKeyPair.EXPECT().Public().Return(&ecdsaKey.PublicKey).AnyTimes()
7787

7888
var kid = uuid.New()
7989

80-
keySetPrefix := "application_specific_prefix."
8190
expectedPrefixedOpenIDConnectKeyName := fmt.Sprintf("%s%s", keySetPrefix, x.OpenIDConnectKeyName)
8291

83-
m := &hsm.KeyManager{
84-
Context: hsmContext,
85-
KeySetPrefix: keySetPrefix,
86-
}
87-
8892
t.Run("case=GenerateAndPersistKeySet", func(t *testing.T) {
8993
privateAttrSet, publicAttrSet := expectedKeyAttributes(t, expectedPrefixedOpenIDConnectKeyName, kid)
9094
hsmContext.EXPECT().FindKeyPairs(gomock.Nil(), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return(nil, nil)
91-
hsmContext.EXPECT().GenerateRSAKeyPairWithAttributes(gomock.Eq(publicAttrSet), gomock.Eq(privateAttrSet), gomock.Eq(4096)).Return(rsaKeyPair, nil)
95+
hsmContext.EXPECT().GenerateRSAKeyPairWithAttributes(gomock.Eq(publicAttrSet), gomock.Eq(privateAttrSet), gomock.Eq(4096)).Return(rsaKeyPair4096, nil)
9296

9397
got, err := m.GenerateAndPersistKeySet(context.TODO(), x.OpenIDConnectKeyName, kid, "RS256", "sig")
9498

9599
assert.NoError(t, err)
96-
expectedKeySet := expectedKeySet(rsaKeyPair, kid, "RS256", "sig")
100+
expectedKeySet := expectedKeySet(rsaKeyPair4096, kid, "RS256", "sig")
97101
if !reflect.DeepEqual(got, expectedKeySet) {
98102
t.Errorf("GenerateAndPersistKeySet() got = %v, want %v", got, expectedKeySet)
99103
}
100104
})
101105
t.Run("case=GetKey", func(t *testing.T) {
102-
hsmContext.EXPECT().FindKeyPair(gomock.Eq([]byte(kid)), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return(rsaKeyPair, nil)
103-
hsmContext.EXPECT().GetAttribute(gomock.Eq(rsaKeyPair), gomock.Eq(crypto11.CkaDecrypt)).Return(nil, nil)
106+
hsmContext.EXPECT().FindKeyPair(gomock.Eq([]byte(kid)), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return(rsaKeyPair4096, nil)
107+
hsmContext.EXPECT().GetAttribute(gomock.Eq(rsaKeyPair4096), gomock.Eq(crypto11.CkaDecrypt)).Return(nil, nil)
104108

105109
got, err := m.GetKey(context.TODO(), x.OpenIDConnectKeyName, kid)
106110

107111
assert.NoError(t, err)
108-
expectedKeySet := expectedKeySet(rsaKeyPair, kid, "RS256", "sig")
112+
expectedKeySet := expectedKeySet(rsaKeyPair4096, kid, "RS256", "sig")
109113
if !reflect.DeepEqual(got, expectedKeySet) {
110114
t.Errorf("GetKey() got = %v, want %v", got, expectedKeySet)
111115
}
112116
})
117+
t.Run("case=GetKeyMinimalRsaKeyLengthError", func(t *testing.T) {
118+
hsmContext.EXPECT().FindKeyPair(gomock.Eq([]byte(kid)), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return(rsaKeyPair3072, nil)
119+
120+
_, err := m.GetKey(context.TODO(), x.OpenIDConnectKeyName, kid)
121+
122+
assert.ErrorIs(t, err, jwk.ErrMinimalRsaKeyLength)
123+
})
113124
t.Run("case=GetKeySet", func(t *testing.T) {
114-
hsmContext.EXPECT().FindKeyPairs(gomock.Nil(), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return([]crypto11.Signer{rsaKeyPair}, nil)
115-
hsmContext.EXPECT().GetAttribute(gomock.Eq(rsaKeyPair), gomock.Eq(crypto11.CkaId)).Return(pkcs11.NewAttribute(pkcs11.CKA_ID, []byte(kid)), nil)
116-
hsmContext.EXPECT().GetAttribute(gomock.Eq(rsaKeyPair), gomock.Eq(crypto11.CkaDecrypt)).Return(nil, nil)
125+
hsmContext.EXPECT().FindKeyPairs(gomock.Nil(), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return([]crypto11.Signer{rsaKeyPair4096}, nil)
126+
hsmContext.EXPECT().GetAttribute(gomock.Eq(rsaKeyPair4096), gomock.Eq(crypto11.CkaId)).Return(pkcs11.NewAttribute(pkcs11.CKA_ID, []byte(kid)), nil)
127+
hsmContext.EXPECT().GetAttribute(gomock.Eq(rsaKeyPair4096), gomock.Eq(crypto11.CkaDecrypt)).Return(nil, nil)
117128

118129
got, err := m.GetKeySet(context.TODO(), x.OpenIDConnectKeyName)
119130

120131
assert.NoError(t, err)
121-
expectedKeySet := expectedKeySet(rsaKeyPair, kid, "RS256", "sig")
132+
expectedKeySet := expectedKeySet(rsaKeyPair4096, kid, "RS256", "sig")
122133
if !reflect.DeepEqual(got, expectedKeySet) {
123134
t.Errorf("GetKey() got = %v, want %v", got, expectedKeySet)
124135
}
125136
})
137+
t.Run("case=GetKeySetMinimalRsaKeyLengthError", func(t *testing.T) {
138+
hsmContext.EXPECT().FindKeyPairs(gomock.Nil(), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return([]crypto11.Signer{rsaKeyPair3072}, nil)
139+
hsmContext.EXPECT().GetAttribute(gomock.Eq(rsaKeyPair3072), gomock.Eq(crypto11.CkaId)).Return(pkcs11.NewAttribute(pkcs11.CKA_ID, []byte(kid)), nil)
140+
141+
_, err := m.GetKeySet(context.TODO(), x.OpenIDConnectKeyName)
142+
143+
assert.ErrorIs(t, err, jwk.ErrMinimalRsaKeyLength)
144+
})
126145
t.Run("case=DeleteKey", func(t *testing.T) {
127-
hsmContext.EXPECT().FindKeyPair(gomock.Eq([]byte(kid)), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return(rsaKeyPair, nil)
128-
rsaKeyPair.EXPECT().Delete().Return(nil)
146+
hsmContext.EXPECT().FindKeyPair(gomock.Eq([]byte(kid)), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return(rsaKeyPair4096, nil)
147+
rsaKeyPair4096.EXPECT().Delete().Return(nil)
129148

130149
err := m.DeleteKey(context.TODO(), x.OpenIDConnectKeyName, kid)
131150

132151
assert.NoError(t, err)
133152
})
134153
t.Run("case=DeleteKeySet", func(t *testing.T) {
135-
hsmContext.EXPECT().FindKeyPairs(gomock.Nil(), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return([]crypto11.Signer{rsaKeyPair}, nil)
136-
rsaKeyPair.EXPECT().Delete().Return(nil)
154+
hsmContext.EXPECT().FindKeyPairs(gomock.Nil(), gomock.Eq([]byte(expectedPrefixedOpenIDConnectKeyName))).Return([]crypto11.Signer{rsaKeyPair4096}, nil)
155+
rsaKeyPair4096.EXPECT().Delete().Return(nil)
137156

138157
err := m.DeleteKeySet(context.TODO(), x.OpenIDConnectKeyName)
139158

@@ -145,8 +164,11 @@ func TestKeyManager_GenerateAndPersistKeySet(t *testing.T) {
145164
ctrl := gomock.NewController(t)
146165
hsmContext := NewMockContext(ctrl)
147166
defer ctrl.Finish()
167+
l := logrusx.New("", "")
168+
c := config.MustNew(context.Background(), l, configx.SkipValidation())
169+
m := hsm.NewKeyManager(hsmContext, c)
148170

149-
rsaKey, err := rsa.GenerateKey(rand.Reader, 512)
171+
rsaKey, err := rsa.GenerateKey(rand.Reader, 4096)
150172
require.NoError(t, err)
151173

152174
ecdsaKey, err := ecdsa.GenerateKey(elliptic.P521(), rand.Reader)
@@ -303,9 +325,6 @@ func TestKeyManager_GenerateAndPersistKeySet(t *testing.T) {
303325
for _, tt := range tests {
304326
t.Run(tt.name, func(t *testing.T) {
305327
tt.setup(t)
306-
m := &hsm.KeyManager{
307-
Context: hsmContext,
308-
}
309328
got, err := m.GenerateAndPersistKeySet(tt.args.ctx, tt.args.set, tt.args.kid, tt.args.alg, tt.args.use)
310329
if tt.wantErr != nil {
311330
require.Nil(t, got)
@@ -326,8 +345,11 @@ func TestKeyManager_GetKey(t *testing.T) {
326345
ctrl := gomock.NewController(t)
327346
hsmContext := NewMockContext(ctrl)
328347
defer ctrl.Finish()
348+
l := logrusx.New("", "")
349+
c := config.MustNew(context.Background(), l, configx.SkipValidation())
350+
m := hsm.NewKeyManager(hsmContext, c)
329351

330-
rsaKey, err := rsa.GenerateKey(rand.Reader, 512)
352+
rsaKey, err := rsa.GenerateKey(rand.Reader, 4096)
331353
require.NoError(t, err)
332354
rsaKeyPair := NewMockSignerDecrypter(ctrl)
333355
rsaKeyPair.EXPECT().Public().Return(&rsaKey.PublicKey).AnyTimes()
@@ -493,9 +515,6 @@ func TestKeyManager_GetKey(t *testing.T) {
493515
for _, tt := range tests {
494516
t.Run(tt.name, func(t *testing.T) {
495517
tt.setup(t)
496-
m := &hsm.KeyManager{
497-
Context: hsmContext,
498-
}
499518
got, err := m.GetKey(tt.args.ctx, tt.args.set, tt.args.kid)
500519
if tt.wantErr != nil {
501520
require.Nil(t, got)
@@ -516,8 +535,11 @@ func TestKeyManager_GetKeySet(t *testing.T) {
516535
ctrl := gomock.NewController(t)
517536
hsmContext := NewMockContext(ctrl)
518537
defer ctrl.Finish()
538+
l := logrusx.New("", "")
539+
c := config.MustNew(context.Background(), l, configx.SkipValidation())
540+
m := hsm.NewKeyManager(hsmContext, c)
519541

520-
rsaKey, err := rsa.GenerateKey(rand.Reader, 512)
542+
rsaKey, err := rsa.GenerateKey(rand.Reader, 4096)
521543
require.NoError(t, err)
522544
rsaKid := uuid.New()
523545
rsaKeyPair := NewMockSignerDecrypter(ctrl)
@@ -641,9 +663,6 @@ func TestKeyManager_GetKeySet(t *testing.T) {
641663
for _, tt := range tests {
642664
t.Run(tt.name, func(t *testing.T) {
643665
tt.setup(t)
644-
m := &hsm.KeyManager{
645-
Context: hsmContext,
646-
}
647666
got, err := m.GetKeySet(tt.args.ctx, tt.args.set)
648667
if tt.wantErr != nil {
649668
require.Nil(t, got)
@@ -664,6 +683,9 @@ func TestKeyManager_DeleteKey(t *testing.T) {
664683
ctrl := gomock.NewController(t)
665684
hsmContext := NewMockContext(ctrl)
666685
defer ctrl.Finish()
686+
l := logrusx.New("", "")
687+
c := config.MustNew(context.Background(), l, configx.SkipValidation())
688+
m := hsm.NewKeyManager(hsmContext, c)
667689

668690
rsaKeyPair := NewMockSignerDecrypter(ctrl)
669691

@@ -733,9 +755,6 @@ func TestKeyManager_DeleteKey(t *testing.T) {
733755
for _, tt := range tests {
734756
t.Run(tt.name, func(t *testing.T) {
735757
tt.setup(t)
736-
m := &hsm.KeyManager{
737-
Context: hsmContext,
738-
}
739758
if err := m.DeleteKey(tt.args.ctx, tt.args.set, tt.args.kid); len(tt.wantErrMsg) != 0 {
740759
require.EqualError(t, err, tt.wantErrMsg)
741760
}
@@ -747,6 +766,9 @@ func TestKeyManager_DeleteKeySet(t *testing.T) {
747766
ctrl := gomock.NewController(t)
748767
hsmContext := NewMockContext(ctrl)
749768
defer ctrl.Finish()
769+
l := logrusx.New("", "")
770+
c := config.MustNew(context.Background(), l, configx.SkipValidation())
771+
m := hsm.NewKeyManager(hsmContext, c)
750772

751773
rsaKeyPair1 := NewMockSignerDecrypter(ctrl)
752774
rsaKeyPair2 := NewMockSignerDecrypter(ctrl)
@@ -812,9 +834,6 @@ func TestKeyManager_DeleteKeySet(t *testing.T) {
812834
for _, tt := range tests {
813835
t.Run(tt.name, func(t *testing.T) {
814836
tt.setup(t)
815-
m := &hsm.KeyManager{
816-
Context: hsmContext,
817-
}
818837
if err := m.DeleteKeySet(tt.args.ctx, tt.args.set); len(tt.wantErrMsg) != 0 {
819838
require.EqualError(t, err, tt.wantErrMsg)
820839
}

‎jwk/manager.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,12 @@ var ErrUnsupportedEllipticCurve = &fosite.RFC6749Error{
2626
DescriptionField: "Unsupported elliptic curve",
2727
}
2828

29+
var ErrMinimalRsaKeyLength = &fosite.RFC6749Error{
30+
CodeField: http.StatusBadRequest,
31+
ErrorField: http.StatusText(http.StatusBadRequest),
32+
DescriptionField: "Unsupported RSA key length",
33+
}
34+
2935
type (
3036
Manager interface {
3137
GenerateAndPersistKeySet(ctx context.Context, set, kid, alg, use string) (*jose.JSONWebKeySet, error)

0 commit comments

Comments
 (0)