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

crypto/x509: add a PKITS test #45857

Open
FiloSottile opened this issue Apr 29, 2021 · 6 comments
Open

crypto/x509: add a PKITS test #45857

FiloSottile opened this issue Apr 29, 2021 · 6 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@FiloSottile
Copy link
Contributor

PKITS is a large X.509 test suite. Some of the stuff in there we don't actually support, but it would be good to run through it all and explicitly mark the things we don't support.

@FiloSottile FiloSottile added Testing An issue that has been verified to require only test changes, not just a test failure. help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Apr 29, 2021
@FiloSottile FiloSottile added this to the Backlog milestone Apr 29, 2021
@dandare100
Copy link

Hello

I have started with this and just want to know if I'm on the right track before carrying on with CRL tests

I understand that you are looking for a list of unsupported tests/functionality and I am assuming these will be fixed/catered for on a case by case basis ?

Are you also looking for a PKITS test in a golang test case format ?

Ran the tests for 4.1.1 through to 4.5.8 for now.

Here are the observations for test failures:

4.1.4

test 4.1.4 [Signature Verification : Valid DSA Signatures Test4]: failed. x509: certificate signed by unknown authority (possibly because of "x509: cannot verify signature: algorithm unimplemented" while trying to verify candidate authority certificate "DSA CA

fails in x509.go
func checkSignature(algo SignatureAlgorithm, signed, signature []byte, publicKey crypto.PublicKey) (err error)
because only rsa, ecdsa and ed25519 public keys are supported
It looks like DSA signature checks are not implemented/supported. Docs say is deprecated but the test requires it

4.1.5

4.1.5 [Signature Verification : Valid DSA Parameter Inheritance Test5] skipping test : failed to generate ca chain asn1: syntax error: sequence truncated

Failed parsing of DSAParametersInheritedCACert.crt in


paramsData := keyData.Algorithm.Parameters.FullBytes
		params := new(dsaAlgorithmParameters)
		rest, err = asn1.Unmarshal(paramsData, params)

paramsData and params are nil

4.1.6

test 4.1.6 [Signature Verification : Invalid DSA Signature Test6]: x509: certificate signed by unknown authority (possibly because of "x509: cannot verify signature: algorithm unimplemented" while trying to verify candidate authority certificate "DSA CA")

fails in x509.go
```func checkSignature(algo SignatureAlgorithm, signed, signature []byte, publicKey crypto.PublicKey) (err error)``

because only rsa, ecdsa and ed25519 public keys are supported
It looks like DSA signature checks are not implemented/supported. Docs say is deprecated but the test requires it

4.3.3,4,5,10,11

test 4.3.3 [Verifying Name Chaining : Valid Name Chaining Whitespace Test3]:  failed. x509: certificate signed by unknown authority
test 4.3.4 [Verifying Name Chaining : Valid Name Chaining Whitespace Test4]:  failed. x509: certificate signed by unknown authority
test 4.3.5 [Verifying Name Chaining : Valid Name Chaining Capitalization Test5]:  failed. x509: certificate signed by unknown authority
test 4.3.10 [Verifying Name Chaining : Valid Rollover from PrintableString to UTF8String Test10]:  failed. x509: certificate signed by unknown authority
test 4.3.11 [Verifying Name Chaining : Valid UTF8String Case Insensitive Match Test11]:  failed. x509: certificate signed by unknown authority

in cert_pool.go

func (s *CertPool) findPotentialParents(cert *Certificate) []*Certificate {

for _, c := range s.byName[string(cert.RawIssuer)] {

The raw issuer does not match raw subject because of spaces, capitilization.
The reason is certs are keyed by string of cert.RawSubject

// AddCert adds a certificate to a pool.

func (s *CertPool) AddCert(cert *Certificate) {
	if cert == nil {
		panic("adding nil Certificate to CertPool")
	}
	s.addCertFunc(sha256.Sum224(cert.Raw), string(cert.RawSubject), func() (*Certificate, error) {
		return cert, nil
	})
}

also in verify.go

if !bytes.Equal(child.RawIssuer, c.RawSubject) {
			return CertificateInvalidError{c, NameMismatch, ""}
		}

Conforming implementations MUST use the LDAP StringPrep profile
(including insignificant space handling), as specified in [RFC4518], (2.6.1)
as the basis for comparison of distinguished name attributes encoded
in either PrintableString or UTF8String. Conforming implementations
MUST support name comparisons using caseIgnoreMatch.

I also suspect that some tests pass (and should) but only a subset of checks are done

For example:

test 4.3.6 [Verifying Name Chaining : Valid Name Chaining UIDs Test6]: passed.

The subject and issuer uids are catered for in the asn.1 certificate struct but not ever copied over to Certificate in the parseCertificate function

in asn.1 certificate struct

UniqueId           asn1.BitString   `asn1:"optional,tag:1"`
SubjectUniqueId    asn1.BitString   `asn1:"optional,tag:2"`
func parseCertificate(in *certificate) (*Certificate, error) {

Is it ok for me to complete the test analysis like the above, package it in a better format when complete or did you have another approach in mind?

@dandare100
Copy link

results.4.1.1-4.5.8.txt

@FiloSottile
Copy link
Contributor Author

Hello @dandare100, thank you for giving this a spin!

I encourage you to send a small CL/PR with the tests you have so far, so we can discuss the approach without reviewing a massive change set. The test should be a standard test in a _test.go file in the crypto/x509 package.

In general, it's expected there will be test cases that PKITS calls valid but we reject. We tend to be stricter than the standard where we can afford it. For those, we want to clearly document in the test what is going on, and check that they fail.

What we don't want is the opposite, invalid chains that we accept.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/339292 mentions this issue: crypto: PKITS tests

@dandare100
Copy link

Hello.
Thank you.
I have submitted a small CL/PR to start the discussion as requested.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/618415 mentions this issue: crypto/x509: run the NIST PKI test suite

gopherbot pushed a commit that referenced this issue Nov 22, 2024
This vendors the vectors (generated using [0], derived from the
BoringSSL script which generates their test headers) and all of the
certs, but only runs the subset of the suite that is focused on policy
validation.

In the future we may want to run more of the suite, since it is focused
on path validation, not path building, the way it interacts with our
hybrid path builder/validator is kind of complicated.

Updates #68484
Updates #45857

[0] https://gist.github.com/rolandshoemaker/a4efa9d65c2cef74a46ea40f47f0729e

Change-Id: Ic04323dcd76aa5cbd6372c8cb1c44ccb91ccbca4
Reviewed-on: https://go-review.googlesource.com/c/go/+/618415
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

3 participants