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 · 5 comments
Open

crypto/x509: add a PKITS test #45857

FiloSottile opened this issue Apr 29, 2021 · 5 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

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.

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