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

Jump to bottom

Use of deprecated sha1 #27

Open
nicopal opened this issue Sep 6, 2024 · 2 comments · May be fixed by #29
Open

Use of deprecated sha1 #27

nicopal opened this issue Sep 6, 2024 · 2 comments · May be fixed by #29
Assignees
@hslatman
Milestone
v2

Comments

@nicopal
Copy link

nicopal commented Sep 6, 2024 •
edited
Loading

Hi,

in the pkcs7 implementation, function NewSignedData in sign.go defaults to using SHA-1 as digest algorithm.
https://github.com/smallstep/pkcs7/blob/5e2c6a136dfaa418340bb4a7eb0d0c7421d4934c/sign.go

// NewSignedData takes data and initializes a PKCS7 SignedData struct that is
// ready to be signed via AddSigner. The digest algorithm is set to SHA1 by default
// and can be changed by calling SetDigestAlgorithm.
func NewSignedData(data []byte) (*SignedData, error) {
	content, err := asn1.Marshal(data)
	if err != nil {
		return nil, err
	}
	ci := contentInfo{
		ContentType: OIDData,
		Content:     asn1.RawValue{Class: 2, Tag: 0, Bytes: content, IsCompound: true},
	}
	sd := signedData{
		ContentInfo: ci,
		Version:     1,
	}
	return &SignedData{sd: sd, data: data, digestOid: OIDDigestAlgorithmSHA1}, nil
}

However, the use of sha1 is unadvisable according to NIST, and implementations should migrate to SHA-2 or SHA-3 as soon as possible (source: https://www.nist.gov/news-events/news/2022/12/nist-retires-sha-1-cryptographic-algorithm)

Is there a good reason this implementation defaults to SHA-1? Alternatively, would you accept a PR updating the default to SHA-2?
@hslatman
Copy link
Member

hslatman commented Sep 9, 2024 •
edited
Loading

Hi @nicopal, thank you for opening this issue. The reason it currently (still) defaults to using SHA1 is for backwards compatibility. If we were to change the default algorithm now, it's possible that systems importing the package do no longer work as a whole.

The current API for providing this type of setting in pkcs7 is lacking, but it might be possible to do something using a package level global, so that it doesn't break systems, and is an intentional change on the importer side. A package level global is not ideal though, and we've been thinking about more improvements along the lines of default algorithms and potentially backwards incompatible changes, so that'll happen at some point.

It seems possible to change it using SetDigestAlgorithm. Maybe we can expose a package global, similar to ContentEncryptionAlgorithm, so that the default can be changed that way, instead of having to call SetDigestAlgorithm every time you're using NewSignedData. Ideally the NewSignedData would take it as configuration instead of relying on a package level variable, but that would be a backwards incompatible change.

@nicopal
Copy link
Author

nicopal commented Sep 10, 2024

Hi @hslatman thank you for looking into this. I suspected backward compatibility was the culprit (it's common). My initial thought was that perhaps NewSignedData could still support SHA1 but at least not default to it. However you have a better view of the constraints/requirements that others have towards this library and exposing the package global so that the default of SetDigestAlgorithm can be changed seems like a good solution.

@hslatman hslatman self-assigned this Sep 10, 2024
@hslatman hslatman linked a pull request Sep 10, 2024 that will close this issue
Add SetDefaultDigestAlgorithm #29
Draft
@stv0g stv0g mentioned this issue Sep 17, 2024
Open
@hslatman hslatman added this to the v2 milestone Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hslatman hslatman

Labels
None yet
Projects
None yet
Milestone
v2
Development

Successfully merging a pull request may close this issue.

Add SetDefaultDigestAlgorithm smallstep/pkcs7
2 participants
@nicopal @hslatman