Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crypto: let Exists() return an error if one occurs #3127

Merged
merged 2 commits into from
May 21, 2024
Merged

Crypto: let Exists() return an error if one occurs #3127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcf7d95
Crypto: let Exists() return an error if one occurs
reinkrul May 18, 2024
5ce860f
wrap error
reinkrul May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion auth/services/notary/notary.go
View file
Open in desktop
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
reinkrul marked this conversation as resolved.
Show resolved Hide resolved
}
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop

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
Open in desktop
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
Loading