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.

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

Abstract out signature generation and verification to SignatureEnvelope #7

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

priteshbandi
Copy link
Contributor

Change Log:

  • Implements SignatureEnvelope struct that would be used by notation-go-lib to generate/verify the signature.
  • SignatureEnvelope internally implements actual signature formats like jws(cose in future)
  • Implemented JWS-JSON signature generation and verification code.

@priteshbandi priteshbandi changed the title Adds shapes and methods for signature related tasks Abstract out signature generation and verification to SignatureEnvelope Jun 14, 2022
internal/testhelper/certificatetest.go Outdated Show resolved Hide resolved
internal/testhelper/certificatetest.go Outdated Show resolved Hide resolved
internal/testhelper/certificatetest.go Outdated Show resolved Hide resolved
internal/testhelper/certificatetest.go Outdated Show resolved Hide resolved
internal/testhelper/certificatetest.go Outdated Show resolved Hide resolved
signer/errors.go Outdated Show resolved Hide resolved
signer/errors.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Show resolved Hide resolved
@priteshbandi priteshbandi force-pushed the core-refactor branch 5 times, most recently from 8236b81 to a33fae5 Compare June 15, 2022 21:19
go.mod Outdated Show resolved Hide resolved
internal/testhelper/certificatetest.go Outdated Show resolved Hide resolved
signer/errors.go Outdated Show resolved Hide resolved
signer/errors.go Outdated Show resolved Hide resolved
signer/errors.go Outdated Show resolved Hide resolved
signer/errors.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
@priteshbandi priteshbandi force-pushed the core-refactor branch 8 times, most recently from 223b44e to cb4fca4 Compare June 19, 2022 17:44
Copy link

@qmuntal qmuntal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comment before doing an in-depth review:

Could you take a look at notaryproject/notation-go#62? I've changes the data model of several notation types to fit the new signing spec and moved away from jwt.Token, which simplifies the encoding/decoding logic. I think this PR would also benefit from taking that approach, as I guess it is using that many maps just because jwt.Token.Header is a map instead of a typed struct.

signer/jws.go Outdated Show resolved Hide resolved
signer/errors.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/signer.go Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/jws.go Outdated Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
signer/signer.go Show resolved Hide resolved
Comment on lines +39 to +63
// GetRSARootCertificate returns root certificate signed using RSA algorithm
func GetRSARootCertificate() CertTuple {
return rsaRoot
}

// GetRSALeafCertificate returns leaf certificate signed using RSA algorithm
func GetRSALeafCertificate() CertTuple {
return rsaLeaf
}

// GetECRootCertificate returns root certificate signed using EC algorithm
func GetECRootCertificate() ECCertTuple {
return ecdsaRoot
}

// GetECLeafCertificate returns leaf certificate signed using EC algorithm
func GetECLeafCertificate() ECCertTuple {
return ecdsaLeaf
}

// GetUnsupportedCertificate returns certificate signed using RSA algorithm with key size of 1024 bits
// which is not supported by notary.
func GetUnsupportedCertificate() CertTuple {
return unsupported
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to define all these getters. Just use the global variables wherever necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having global gives less control than the function, because anyone can read or change the global variable at any time. Also, having function will allow us to change variable name/representation internally without affecting callers.

signer/jws.go Outdated Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
signer/jws_test.go Outdated Show resolved Hide resolved
signer/jws_test.go Outdated Show resolved Hide resolved
signer/jws_test.go Show resolved Hide resolved
signer/jws_test.go Outdated Show resolved Hide resolved
signer/jws_test.go Outdated Show resolved Hide resolved
signer/jws_test.go Outdated Show resolved Hide resolved
@priteshbandi priteshbandi force-pushed the core-refactor branch 2 times, most recently from 92f5064 to f03bbc8 Compare June 24, 2022 18:18
signer/signer_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rgnote rgnote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

signer/errors.go Show resolved Hide resolved
signer/jwt.go Outdated Show resolved Hide resolved
signer/jwt.go Outdated Show resolved Hide resolved
Sign([]byte) ([]byte, error)
}

type SignatureEnvelope struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make SignatureEnvelope an interface lile

type SignatureEnvelope interface {
    Sign(req SignRequest) ([]byte, error)
    Verify(trustedCerts []*x509.Certificate) (*x509.Certificate, *SignerInfo, error)
    GetSignerInfo() (*SignerInfo, error)
}

Then we can have stuffs simplified like

// NewSignatureEnvelope is used for signature generation workflow
func NewSignatureEnvelope(envelopeMediaType SignatureMediaType) (SignatureEnvelope, error) {
	switch envelopeMediaType {
	case MediaTypeJWSJSON:
		return &jwsEnvelope{}, nil
	case MediaTypeCOSE:
		return &coseEnvelope{}, nil
	}
	return nil, &ErrorUnsupportedSignatureFormat{mediaType: string(envelopeMediaType)}
}

Copy link
Contributor Author

@priteshbandi priteshbandi Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically we want to implement common validations(like input validations, cert chain validations, trust anchor related code, timestamping related code) in SignatureEnvelope's sign and verify method and let specific implementation only do the work of encoding/decoding/integrity validation signature format.
With the interface above things are not possible. Although there can be a workaround by creating helper methods then we need to make sure each implementation is calling them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we can still have

// NewSignatureEnvelope is used for signature generation workflow
func NewSignatureEnvelope(envelopeMediaType SignatureMediaType) (SignatureEnvelope, error) {
	switch envelopeMediaType {
	case MediaTypeJWSJSON:
		return &someGenericValidation{&jwsEnvelope{}}, nil
	case MediaTypeCOSE:
		return &someGenericValidation{&coseEnvelope{}}, nil
	}
	return nil, &UnsupportedSignatureFormatError{mediaType: string(envelopeMediaType)}
}

That still works. With interfaces, it is easier to decouple code and do unit testing.

@priteshbandi priteshbandi force-pushed the core-refactor branch 3 times, most recently from a4a21be to 8b4ea2e Compare June 28, 2022 00:22
signer/jwt.go Outdated Show resolved Hide resolved
signer/jwt.go Outdated Show resolved Hide resolved
signer/jwt.go Outdated Show resolved Hide resolved
signer/signer.go Show resolved Hide resolved
signer/signer.go Show resolved Hide resolved
Sign([]byte) ([]byte, error)
}

type SignatureEnvelope struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we can still have

// NewSignatureEnvelope is used for signature generation workflow
func NewSignatureEnvelope(envelopeMediaType SignatureMediaType) (SignatureEnvelope, error) {
	switch envelopeMediaType {
	case MediaTypeJWSJSON:
		return &someGenericValidation{&jwsEnvelope{}}, nil
	case MediaTypeCOSE:
		return &someGenericValidation{&coseEnvelope{}}, nil
	}
	return nil, &UnsupportedSignatureFormatError{mediaType: string(envelopeMediaType)}
}

That still works. With interfaces, it is easier to decouple code and do unit testing.

signer/signer.go Outdated Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
signer/signer.go Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


// Verify performs integrity and other signature specification related validations
// Returns the SignerInfo object containing the information about the signature.
func (s *SignatureEnvelope) Verify() (*SignerInfo, error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo singerInfo, can be addressed in subsequent PR.

@gokarnm
Copy link

gokarnm commented Jun 29, 2022

LGTM!

// ***********************************************************************
const (
// PayloadContentTypeJWSV1 describes the media type of the jwsEnvelope envelope.
PayloadContentTypeJWSV1 = "application/vnd.cncf.notary.v2.jws.v1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the Notary v2 payload type, it's signature format agnostic and should be set to application/vnd.cncf.notary.payload.v1+json

https://github.com/notaryproject/notaryproject/blob/main/signature-specification.md#payload

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants