Skip to content

Commit

Permalink
Crypto: let Exists() return an error if one occurs (#3127)
Browse files Browse the repository at this point in the history 
* Crypto: let Exists() return an error if one occurs

* wrap error
  • Loading branch information
@reinkrul
reinkrul authored May 21, 2024
1 parent 3d8ed7b commit ee3b18f
Show file tree
Hide file tree
Showing 22 changed files with 149 additions and 74 deletions.
6 changes: 5 additions & 1 deletion auth/services/notary/notary.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,11 @@ func (n *notary) DrawUpContract(ctx context.Context, template contract.Template,
return nil, fmt.Errorf("could not draw up contract: %w", err)
}

if !n.privateKeyStore.Exists(ctx, signingKeyID.String()) {
exists, err := n.privateKeyStore.Exists(ctx, signingKeyID.String())
if err != nil {
return nil, err
}
if !exists {
return nil, services.InvalidContractRequestError{Message: fmt.Errorf("organization is not managed by this node: %w", ErrMissingOrganizationKey)}
}

Expand Down
20 changes: 10 additions & 10 deletions auth/services/notary/notary_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestContract_DrawUpContract(t *testing.T) {
test := buildContext(t)

test.keyResolver.EXPECT().ResolveKey(orgID, &validFrom, resolver.NutsSigningKeyType).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true, nil)
test.vcr.EXPECT().Search(context.Background(), searchTerms, false, nil).Return([]vc.VerifiableCredential{testCredential}, nil)

drawnUpContract, err := test.notary.DrawUpContract(ctx, template, orgID, validFrom, duration, nil)
Expand All @@ -83,7 +83,7 @@ func TestContract_DrawUpContract(t *testing.T) {
defer test.ctrl.Finish()

test.keyResolver.EXPECT().ResolveKey(orgID, gomock.Any(), resolver.NutsSigningKeyType).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true, nil)

drawnUpContract, err := test.notary.DrawUpContract(ctx, template, orgID, validFrom, duration, &testCredential)
require.NoError(t, err)
Expand All @@ -96,7 +96,7 @@ func TestContract_DrawUpContract(t *testing.T) {
test := buildContext(t)

test.keyResolver.EXPECT().ResolveKey(orgID, &validFrom, gomock.Any()).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true, nil)
test.vcr.EXPECT().Search(context.Background(), searchTerms, false, nil).Return([]vc.VerifiableCredential{testCredential}, nil)

drawnUpContract, err := test.notary.DrawUpContract(ctx, template, orgID, validFrom, 0, nil)
Expand All @@ -110,7 +110,7 @@ func TestContract_DrawUpContract(t *testing.T) {
test := buildContext(t)

test.keyResolver.EXPECT().ResolveKey(orgID, &time.Time{}, gomock.Any()).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true, nil)
test.vcr.EXPECT().Search(context.Background(), searchTerms, false, nil).Return([]vc.VerifiableCredential{testCredential}, nil)

timeNow = func() time.Time {
Expand Down Expand Up @@ -139,7 +139,7 @@ func TestContract_DrawUpContract(t *testing.T) {
test := buildContext(t)

test.keyResolver.EXPECT().ResolveKey(orgID, &validFrom, gomock.Any()).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(false)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(false, nil)

drawnUpContract, err := test.notary.DrawUpContract(ctx, template, orgID, validFrom, duration, nil)

Expand All @@ -162,7 +162,7 @@ func TestContract_DrawUpContract(t *testing.T) {
test := buildContext(t)

test.keyResolver.EXPECT().ResolveKey(orgID, &validFrom, gomock.Any()).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true, nil)
test.vcr.EXPECT().Search(context.Background(), searchTerms, false, nil).Return(nil, errors.New("error occurred"))

drawnUpContract, err := test.notary.DrawUpContract(ctx, template, orgID, validFrom, duration, nil)
Expand All @@ -175,7 +175,7 @@ func TestContract_DrawUpContract(t *testing.T) {
test := buildContext(t)

test.keyResolver.EXPECT().ResolveKey(orgID, &validFrom, gomock.Any()).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true, nil)
test.vcr.EXPECT().Search(context.Background(), searchTerms, false, nil).Return([]vc.VerifiableCredential{testCredential}, nil)

template := contract.Template{
Expand All @@ -192,7 +192,7 @@ func TestContract_DrawUpContract(t *testing.T) {
test := buildContext(t)

test.keyResolver.EXPECT().ResolveKey(orgID, &validFrom, gomock.Any()).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true, nil)
test.vcr.EXPECT().Search(context.Background(), searchTerms, false, nil).Return([]vc.VerifiableCredential{testCredential, testCredential}, nil)

drawnUpContract, err := test.notary.DrawUpContract(ctx, template, orgID, validFrom, duration, nil)
Expand All @@ -209,7 +209,7 @@ func TestContract_DrawUpContract(t *testing.T) {
_ = json.Unmarshal([]byte(jsonld.TestCredential), &testCredential2)

test.keyResolver.EXPECT().ResolveKey(orgID, &validFrom, gomock.Any()).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true, nil)
test.vcr.EXPECT().Search(context.Background(), searchTerms, false, nil).Return([]vc.VerifiableCredential{testCredential, testCredential2}, nil)

drawnUpContract, err := test.notary.DrawUpContract(ctx, template, orgID, validFrom, duration, nil)
Expand All @@ -222,7 +222,7 @@ func TestContract_DrawUpContract(t *testing.T) {
test := buildContext(t)

test.keyResolver.EXPECT().ResolveKey(orgID, &validFrom, gomock.Any()).Return(keyID, nil, nil)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true)
test.keyStore.EXPECT().Exists(ctx, keyID.String()).Return(true, nil)

drawnUpContract, err := test.notary.DrawUpContract(ctx, template, orgID, validFrom, duration, &vc.VerifiableCredential{})

Expand Down
12 changes: 10 additions & 2 deletions auth/services/oauth/authz_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,11 @@ func (s *authzServer) validateSubject(ctx context.Context, validationCtx *valida
if err != nil {
return err
}
if !s.privateKeyStore.Exists(ctx, signingKeyID.String()) {
exists, err := s.privateKeyStore.Exists(ctx, signingKeyID.String())
if err != nil {
return fmt.Errorf("could not check if JWT signing key exists: %w", err)
}
if !exists {
return fmt.Errorf("subject.vendor: %s is not managed by this node", subject)
}

Expand Down Expand Up @@ -487,7 +491,11 @@ func (s *authzServer) parseAndValidateJwtBearerToken(context *validationContext)
// IntrospectAccessToken fills the fields in NutsAccessToken from the given Jwt Access Token
func (s *authzServer) IntrospectAccessToken(ctx context.Context, accessToken string) (*services.NutsAccessToken, error) {
token, err := nutsCrypto.ParseJWT(accessToken, func(kid string) (crypto.PublicKey, error) {
if !s.privateKeyStore.Exists(ctx, kid) {
exists, err := s.privateKeyStore.Exists(ctx, kid)
if err != nil {
return nil, fmt.Errorf("could not check if JWT signing key exists: %w", err)
}
if !exists {
return nil, fmt.Errorf("JWT signing key not present on this node (kid=%s)", kid)
}
return s.keyResolver.ResolveKeyByID(kid, nil, resolver.NutsSigningKeyType)
Expand Down
22 changes: 11 additions & 11 deletions auth/services/oauth/authz_server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func TestAuth_CreateAccessToken(t *testing.T) {
ctx.contractNotary.EXPECT().VerifyVP(gomock.Any(), nil).Return(nil, errors.New("identity validation failed"))
ctx.keyResolver.EXPECT().ResolveKeyByID(requesterSigningKeyID.String(), gomock.Any(), resolver.NutsSigningKeyType).MinTimes(1).Return(requesterSigningKey.Public(), nil)
ctx.keyResolver.EXPECT().ResolveKey(authorizerDID, gomock.Any(), resolver.NutsSigningKeyType).MinTimes(1).Return(authorizerSigningKeyID, authorizerSigningKey, nil)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true, nil)
tokenCtx := validContext(t)
signToken(tokenCtx)

Expand Down Expand Up @@ -158,7 +158,7 @@ func TestAuth_CreateAccessToken(t *testing.T) {
ctx := createContext(t)

ctx.nameResolver.EXPECT().Search(context.Background(), searchTerms, false, gomock.Any()).Return([]vc.VerifiableCredential{testCredential}, nil)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true, nil)
ctx.keyResolver.EXPECT().ResolveKeyByID(requesterSigningKeyID.String(), gomock.Any(), resolver.NutsSigningKeyType).MinTimes(1).Return(requesterSigningKey.Public(), nil)
ctx.keyResolver.EXPECT().ResolveKey(authorizerDID, gomock.Any(), resolver.NutsSigningKeyType).MinTimes(1).Return(authorizerSigningKeyID, authorizerSigningKey, nil)
ctx.contractNotary.EXPECT().VerifyVP(gomock.Any(), nil).Return(services.TestVPVerificationResult{Val: contract.Invalid, FailureReason: "because of reasons"}, nil)
Expand All @@ -180,7 +180,7 @@ func TestAuth_CreateAccessToken(t *testing.T) {
ctx.nameResolver.EXPECT().Search(context.Background(), searchTerms, false, gomock.Any()).Return([]vc.VerifiableCredential{testCredential}, nil).AnyTimes()
ctx.didResolver.EXPECT().Resolve(authorizerDID, gomock.Any()).Return(getAuthorizerDIDDocument(), nil, nil).AnyTimes()
ctx.serviceResolver.EXPECT().GetCompoundServiceEndpoint(authorizerDID, expectedService, services.OAuthEndpointType, true).Return(expectedAudience, nil).AnyTimes()
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true).AnyTimes()
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true, nil).AnyTimes()
ctx.verifier.EXPECT().Verify(gomock.Any(), true, true, gomock.Any()).Return(nil).AnyTimes()
ctx.contractNotary.EXPECT().VerifyVP(gomock.Any(), nil).Return(services.TestVPVerificationResult{
Val: contract.Valid,
Expand Down Expand Up @@ -226,7 +226,7 @@ func TestAuth_CreateAccessToken(t *testing.T) {
testCtx.nameResolver.EXPECT().Search(context.Background(), searchTerms, false, gomock.Any()).Return([]vc.VerifiableCredential{testCredential}, nil)
testCtx.didResolver.EXPECT().Resolve(authorizerDID, gomock.Any()).Return(getAuthorizerDIDDocument(), nil, nil).AnyTimes()
testCtx.serviceResolver.EXPECT().GetCompoundServiceEndpoint(authorizerDID, expectedService, services.OAuthEndpointType, true).Return(expectedAudience, nil)
testCtx.keyStore.EXPECT().Exists(testCtx.audit, authorizerSigningKeyID.String()).Return(true)
testCtx.keyStore.EXPECT().Exists(testCtx.audit, authorizerSigningKeyID.String()).Return(true, nil)
testCtx.keyStore.EXPECT().SignJWT(gomock.Any(), gomock.Any(), nil, authorizerSigningKeyID.String()).Return("expectedAccessToken", nil)
testCtx.verifier.EXPECT().Verify(gomock.Any(), true, true, gomock.Any()).Return(nil)

Expand All @@ -248,7 +248,7 @@ func TestAuth_CreateAccessToken(t *testing.T) {
ctx.nameResolver.EXPECT().Search(context.Background(), searchTerms, false, gomock.Any()).Return([]vc.VerifiableCredential{testCredential}, nil)
ctx.didResolver.EXPECT().Resolve(authorizerDID, gomock.Any()).Return(getAuthorizerDIDDocument(), nil, nil).AnyTimes()
ctx.serviceResolver.EXPECT().GetCompoundServiceEndpoint(authorizerDID, expectedService, services.OAuthEndpointType, true).Return(expectedAudience, nil)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true, nil)
ctx.keyStore.EXPECT().SignJWT(gomock.Any(), gomock.Any(), nil, authorizerSigningKeyID.String()).Return("expectedAT", nil)
ctx.contractNotary.EXPECT().VerifyVP(gomock.Any(), nil).Return(services.TestVPVerificationResult{
Val: contract.Valid,
Expand Down Expand Up @@ -361,7 +361,7 @@ func TestService_validateSubject(t *testing.T) {
tokenCtx.jwtBearerToken.Set(jwt.SubjectKey, authorizerDID.String())

ctx.keyResolver.EXPECT().ResolveKey(authorizerDID, gomock.Any(), resolver.NutsSigningKeyType).MinTimes(1).Return(authorizerSigningKeyID, authorizerSigningKey, nil)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(true, nil)

err := ctx.oauthService.validateSubject(ctx.audit, tokenCtx)
assert.NoError(t, err)
Expand Down Expand Up @@ -391,7 +391,7 @@ func TestService_validateSubject(t *testing.T) {
tokenCtx.jwtBearerToken.Set(jwt.SubjectKey, authorizerDID.String())

ctx.keyResolver.EXPECT().ResolveKey(authorizerDID, gomock.Any(), resolver.NutsSigningKeyType).MinTimes(1).Return(authorizerSigningKeyID, authorizerSigningKey, nil)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(false)
ctx.keyStore.EXPECT().Exists(ctx.audit, authorizerSigningKeyID.String()).Return(false, nil)

err := ctx.oauthService.validateSubject(ctx.audit, tokenCtx)

Expand Down Expand Up @@ -685,7 +685,7 @@ func TestService_IntrospectAccessToken(t *testing.T) {
ctx := createContext(t)

ctx.keyResolver.EXPECT().ResolveKeyByID(requesterSigningKeyID.String(), nil, resolver.NutsSigningKeyType).MinTimes(1).Return(requesterSigningKey.Public(), nil)
ctx.keyStore.EXPECT().Exists(ctx.audit, requesterSigningKeyID.String()).Return(true)
ctx.keyStore.EXPECT().Exists(ctx.audit, requesterSigningKeyID.String()).Return(true, nil)

// First build an access token
tokenCtx := validAccessToken()
Expand All @@ -706,7 +706,7 @@ func TestService_IntrospectAccessToken(t *testing.T) {
ctx := createContext(t)

ctx.keyResolver.EXPECT().ResolveKeyByID(requesterSigningKeyID.String(), nil, resolver.NutsSigningKeyType).MinTimes(1).Return(requesterSigningKey.Public(), nil)
ctx.keyStore.EXPECT().Exists(ctx.audit, requesterSigningKeyID.String()).Return(true)
ctx.keyStore.EXPECT().Exists(ctx.audit, requesterSigningKeyID.String()).Return(true, nil)

// First build an access token
tokenCtx := validAccessToken()
Expand All @@ -723,7 +723,7 @@ func TestService_IntrospectAccessToken(t *testing.T) {
t.Run("private key not present", func(t *testing.T) {
ctx := createContext(t)

ctx.keyStore.EXPECT().Exists(ctx.audit, requesterSigningKeyID.String()).Return(false)
ctx.keyStore.EXPECT().Exists(ctx.audit, requesterSigningKeyID.String()).Return(false, nil)

// First build an access token
tokenCtx := validContext(t)
Expand All @@ -737,7 +737,7 @@ func TestService_IntrospectAccessToken(t *testing.T) {
t.Run("key not present on DID", func(t *testing.T) {
ctx := createContext(t)

ctx.keyStore.EXPECT().Exists(ctx.audit, requesterSigningKeyID.String()).Return(true)
ctx.keyStore.EXPECT().Exists(ctx.audit, requesterSigningKeyID.String()).Return(true, nil)
ctx.keyResolver.EXPECT().ResolveKeyByID(requesterSigningKeyID.String(), nil, resolver.NutsSigningKeyType).MinTimes(1).Return(nil, resolver.ErrNotFound)

// First build an access token
Expand Down
14 changes: 11 additions & 3 deletions crypto/crypto.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,11 @@ func (client *Crypto) New(ctx context.Context, namingFunc KIDNamingFunc) (Key, e
}

audit.Log(ctx, log.Logger(), audit.CryptoNewKeyEvent).Infof("Generating new key pair: %s", kid)
if client.storage.PrivateKeyExists(ctx, kid) {
exists, err := client.storage.PrivateKeyExists(ctx, kid)
if err != nil {
return nil, err
}
if exists {
return nil, errors.New("key with the given ID already exists")
}
if err = client.storage.SavePrivateKey(ctx, kid, keyPair); err != nil {
Expand Down Expand Up @@ -212,8 +216,12 @@ func GenerateJWK() (jwk.Key, error) {
}

// Exists checks storage for an entry for the given legal entity and returns true if it exists
func (client *Crypto) Exists(ctx context.Context, kid string) bool {
return client.storage.PrivateKeyExists(ctx, kid)
func (client *Crypto) Exists(ctx context.Context, kid string) (bool, error) {
exists, err := client.storage.PrivateKeyExists(ctx, kid)
if err != nil {
return false, fmt.Errorf("could not check if private key exists: %w", err)
}
return exists, nil
}

func (client *Crypto) Resolve(ctx context.Context, kid string) (Key, error) {
Expand Down
16 changes: 11 additions & 5 deletions crypto/crypto_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,21 @@ func TestCrypto_Exists(t *testing.T) {
require.NoError(t, err)

t.Run("returns true for existing key", func(t *testing.T) {
assert.True(t, client.Exists(ctx, kid))
exists, err := client.Exists(ctx, kid)
require.NoError(t, err)
assert.True(t, exists)
})

t.Run("returns false for non-existing key", func(t *testing.T) {
assert.False(t, client.Exists(ctx, "unknown"))
exists, err := client.Exists(ctx, "unknown")
require.NoError(t, err)
assert.False(t, exists)
})

t.Run("returns false for invalid kid", func(t *testing.T) {
assert.False(t, client.Exists(ctx, "../"))
exists, err := client.Exists(ctx, "../")
require.Error(t, err)
assert.False(t, exists)
})
}

Expand Down Expand Up @@ -98,7 +104,7 @@ func TestCrypto_New(t *testing.T) {
t.Run("error - save public key returns an error", func(t *testing.T) {
ctrl := gomock.NewController(t)
storageMock := spi.NewMockStorage(ctrl)
storageMock.EXPECT().PrivateKeyExists(ctx, "123").Return(false)
storageMock.EXPECT().PrivateKeyExists(ctx, "123").Return(false, nil)
storageMock.EXPECT().SavePrivateKey(ctx, gomock.Any(), gomock.Any()).Return(errors.New("foo"))

client := &Crypto{storage: storageMock}
Expand All @@ -111,7 +117,7 @@ func TestCrypto_New(t *testing.T) {
t.Run("error - ID already in use", func(t *testing.T) {
ctrl := gomock.NewController(t)
storageMock := spi.NewMockStorage(ctrl)
storageMock.EXPECT().PrivateKeyExists(ctx, "123").Return(true)
storageMock.EXPECT().PrivateKeyExists(ctx, "123").Return(true, nil)

client := &Crypto{storage: storageMock}
key, err := client.New(ctx, StringNamingFunc("123"))
Expand Down
2 changes: 1 addition & 1 deletion crypto/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ type KeyCreator interface {
type KeyResolver interface {
// Exists returns if the specified private key exists.
// If an error occurs, false is also returned
Exists(ctx context.Context, kid string) bool
Exists(ctx context.Context, kid string) (bool, error)
// Resolve returns a Key for the given KID. ErrPrivateKeyNotFound is returned for an unknown KID.
Resolve(ctx context.Context, kid string) (Key, error)
// List returns the KIDs of the private keys that are present in the KeyStore.
Expand Down
10 changes: 6 additions & 4 deletions crypto/mock.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 8 additions & 3 deletions crypto/storage/external/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,17 @@ func (c APIClient) GetPrivateKey(ctx context.Context, kid string) (crypto.Signer
}
}

func (c APIClient) PrivateKeyExists(ctx context.Context, kid string) bool {
func (c APIClient) PrivateKeyExists(ctx context.Context, kid string) (bool, error) {
response, err := c.httpClient.LookupSecretWithResponse(ctx, url.PathEscape(kid))
if err != nil {
return false
return false, err
}
return response.StatusCode() == http.StatusOK
if response.StatusCode() == http.StatusOK {
return true, nil
} else if response.StatusCode() == http.StatusNotFound {
return false, nil
}
return false, fmt.Errorf("unable to check if private key exists: server returned HTTP %d", response.StatusCode())
}

func (c APIClient) SavePrivateKey(ctx context.Context, kid string, key crypto.PrivateKey) error {
Expand Down
Loading

0 comments on commit ee3b18f

Please sign in to comment.