Skip to content

Conversation

@Yannic
Copy link

@Yannic Yannic commented Dec 23, 2016

No description provided.

pkcs7/sign.go Outdated
// Create Signature of message
// message must be of type io.Reader or []byte
// Returns signature and any error encountered.
func Sign(message interface{}, certificate *x509.Certificate, privateKey *rsa.PrivateKey) ([]byte, error) {
Copy link

Choose a reason for hiding this comment

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

I don't think message should be changed to interface{}.

Instead, I'd suggest that Sign continues using io.Reader and the calling application can use bytes.NewReader() if it needs to pass in []bytes.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you that interface{} is not the best solution.

Instead of using io.Reader I suggest to introduce an interface Hashable Yannic@77cdc19

Copy link

Choose a reason for hiding this comment

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

I'm not clear on how this would work. Do you intend to change Sign to accept Hashable instead of io.Reader then?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think replacing is an option because it wouldn't be compatible with the current version.

func Sign(reader io.Reader, certificate *x509.Certificate, privateKey *rsa.PrivateKey) ([]byte, error) {}

func SignIntermediate(reader io.Reader, certificate *x509.Certificate, privateKey *rsa.PrivateKey, intermediateCertificates []*x509.Certificate) ([]byte, error) {}

func SignData(hashable Hashable, certificate *x509.Certificate, privateKey *rsa.PrivateKey) ([]byte, error) {}

func SignDataIntermediate(hashable Hashable, certificate *x509.Certificate, privateKey *rsa.PrivateKey, intermediateCertificates []*x509.Certificate) ([]byte, error) {}

Copy link

@nathany nathany Jan 9, 2017

Choose a reason for hiding this comment

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

Agreed. But I'm not sure if I see the advantage of of Hashable over using bytes.NewReader() from the std library? Maybe I just need to see an example of how it would be used.

Copy link
Author

Choose a reason for hiding this comment

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

You are right that there is no advantage when using []byte, but someone may have some kind of type struct{} he needs to sign. With Hashable, he has the opportunity to compute a checksum without having to serialize it first.

pkcs7/sign.go Outdated
signedData := SignedData{
// Copy intermediateCertificates to certificate stack
raw := certificate.Raw
if intermediateCertificates != nil {
Copy link

@nathany nathany Jan 7, 2017

Choose a reason for hiding this comment

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

This nil check is unnecessary:
https://play.golang.org/p/4AJfaL9rNu

For more on nil, check out https://www.youtube.com/watch?v=ynoY2xz-F8s

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your explanation, I will remove it.

@Yannic
Copy link
Author

Yannic commented Feb 21, 2017

Ping?

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.

2 participants