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

fix(lib): Adds missing context block to CSR #33

Merged
merged 1 commit into from
May 22, 2020

Conversation

thomastaylor312
Copy link
Contributor

When comparing a CSR generated with the same parameters using openssl,
I noticed that the CSR generated by rcgen was missing attributes data.

OpenSSL:

Certificate Request:
    Data:
        Version: 0 (0x0)
        Subject: C=US, ST=., L=., O=., OU=., CN=krustlet
        Subject Public Key Info:
            Public Key Algorithm: id-ecPublicKey
                Public-Key: (256 bit)
                pub:
                    04:3c:2b:64:c2:0e:8f:fa:86:b2:5c:f1:66:82:a7:
                    73:48:37:3b:d7:f0:12:25:0e:29:c2:65:20:1f:a5:
                    60:36:52:70:ba:60:7e:00:e0:92:74:fe:dd:0f:8a:
                    28:0a:6e:1b:41:46:10:96:72:54:5f:3c:d1:e5:86:
                    59:9b:61:a1:e3
                ASN1 OID: prime256v1
                NIST CURVE: P-256
        Attributes:
            a0:00
    Signature Algorithm: ecdsa-with-SHA256
         30:45:02:20:50:a9:e6:11:9c:46:4b:c2:85:74:f9:58:30:26:
         4f:9c:d2:18:5f:33:fb:b6:74:44:3b:47:b1:2c:e8:cc:14:ac:
         02:21:00:eb:92:eb:b8:98:89:2e:d2:0e:fe:9c:af:c7:fe:60:
         e5:56:af:92:06:37:0f:71:38:1d:19:ba:04:88:3d:b8:b3

rcgen:

Certificate Request:
    Data:
        Version: 0 (0x0)
        Subject: C=US, ST=., L=., O=., OU=., CN=krustlet
        Subject Public Key Info:
            Public Key Algorithm: id-ecPublicKey
                Public-Key: (256 bit)
                pub:
                    04:02:79:50:53:29:21:16:25:a2:bf:f7:24:41:0d:
                    12:7b:bf:60:96:b2:b7:fc:c0:2a:8a:16:69:bc:7e:
                    2d:95:b1:c8:75:23:07:d5:35:89:14:be:56:d3:6b:
                    2f:b1:e5:1a:39:be:74:bb:25:c8:f8:4a:6f:cf:9a:
                    27:39:2c:96:66
                ASN1 OID: prime256v1
                NIST CURVE: P-256
        Attributes:
    Signature Algorithm: ecdsa-with-SHA256
         30:45:02:20:7e:96:16:a3:b3:8b:21:25:ce:a7:8a:e0:53:19:
         cb:3e:24:02:25:73:6a:c8:e7:a0:15:3f:96:c4:b6:bc:50:24:
         02:21:00:b6:0e:d8:5d:1c:76:32:78:b7:0a:da:24:c3:58:3a:
         4d:9c:8e:16:e8:5e:d3:13:61:ec:e0:2c:77:48:b7:d1:23

This turned out to be missing the following section when looking at
openssl asn1parse:

183:d=2  hl=2 l=   0 cons:   cont [ 0 ]

To fix this, I changed the writing of the CSR to always write the context
tag, even if no extended attributes were given.

NOTE: I found this while trying to use the generated CSR with Kubernetes, and
apparently the Go library Kubernetes uses didn't like it. However, upon further investigation,
empty attributes still expect an empty context to be set, so this appears to
be compliant with the spec

@est31
Copy link
Member

est31 commented May 22, 2020

Hmm yeah our behaviour is non-compliant with the spec here. I've checked RFC 2986 appendix A. here the CertificationRequestInfo type's attribute member is tagged with [0] and non-optional.

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

Good catch, thank you. One minor nit before merging.

src/lib.rs Outdated
});
}
// Write extensions
// To match what openssl outputs and what some asn1 decoders expect, always write the
Copy link
Member

Choose a reason for hiding this comment

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

In this instance it's even a spec violation, so could you remove the openssl and decoder references from this comment and instead say that it's expected by the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All fixed!

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure? I'm still seeing the old version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, oops. This is what happens when you forget to git add. Now we're good

When comparing a CSR generated with the same parameters using openssl,
I noticed that the CSR generated by rcgen was missing attributes data.

OpenSSL:
```
Certificate Request:
    Data:
        Version: 0 (0x0)
        Subject: C=US, ST=., L=., O=., OU=., CN=krustlet
        Subject Public Key Info:
            Public Key Algorithm: id-ecPublicKey
                Public-Key: (256 bit)
                pub:
                    04:3c:2b:64:c2:0e:8f:fa:86:b2:5c:f1:66:82:a7:
                    73:48:37:3b:d7:f0:12:25:0e:29:c2:65:20:1f:a5:
                    60:36:52:70:ba:60:7e:00:e0:92:74:fe:dd:0f:8a:
                    28:0a:6e:1b:41:46:10:96:72:54:5f:3c:d1:e5:86:
                    59:9b:61:a1:e3
                ASN1 OID: prime256v1
                NIST CURVE: P-256
        Attributes:
            a0:00
    Signature Algorithm: ecdsa-with-SHA256
         30:45:02:20:50:a9:e6:11:9c:46:4b:c2:85:74:f9:58:30:26:
         4f:9c:d2:18:5f:33:fb:b6:74:44:3b:47:b1:2c:e8:cc:14:ac:
         02:21:00:eb:92:eb:b8:98:89:2e:d2:0e:fe:9c:af:c7:fe:60:
         e5:56:af:92:06:37:0f:71:38:1d:19:ba:04:88:3d:b8:b3
```

rcgen:
```
Certificate Request:
    Data:
        Version: 0 (0x0)
        Subject: C=US, ST=., L=., O=., OU=., CN=krustlet
        Subject Public Key Info:
            Public Key Algorithm: id-ecPublicKey
                Public-Key: (256 bit)
                pub:
                    04:02:79:50:53:29:21:16:25:a2:bf:f7:24:41:0d:
                    12:7b:bf:60:96:b2:b7:fc:c0:2a:8a:16:69:bc:7e:
                    2d:95:b1:c8:75:23:07:d5:35:89:14:be:56:d3:6b:
                    2f:b1:e5:1a:39:be:74:bb:25:c8:f8:4a:6f:cf:9a:
                    27:39:2c:96:66
                ASN1 OID: prime256v1
                NIST CURVE: P-256
        Attributes:
    Signature Algorithm: ecdsa-with-SHA256
         30:45:02:20:7e:96:16:a3:b3:8b:21:25:ce:a7:8a:e0:53:19:
         cb:3e:24:02:25:73:6a:c8:e7:a0:15:3f:96:c4:b6:bc:50:24:
         02:21:00:b6:0e:d8:5d:1c:76:32:78:b7:0a:da:24:c3:58:3a:
         4d:9c:8e:16:e8:5e:d3:13:61:ec:e0:2c:77:48:b7:d1:23
```

This turned out to be missing the following section when looking at
`openssl asn1parse`:

```
183:d=2  hl=2 l=   0 cons:   cont [ 0 ]
```

To fix this, I changed the writing of the CSR to always write the context
tag, even if no extended attributes were given.

NOTE: I found this while trying to use the generated CSR with Kubernetes, and
apparently the Go library didn't like it. However, upon further investigation,
empty attributes still expect an empty context to be set, so this appears to
be compliant with the spec
Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

Thanks!

@est31 est31 merged commit 3bcc02d into rustls:master May 22, 2020
@est31
Copy link
Member

est31 commented May 22, 2020

On a related note, I've been wondering how to add the go TLS implementation to the testsuite.

@thomastaylor312 thomastaylor312 deleted the fix/missing_context_section branch May 22, 2020 17:45
@thomastaylor312
Copy link
Contributor Author

Because go binaries are static, you could build a binary for testing that you can pass the certs, CSRs, etc and include that binary in the tests directory. You could also do a bindgen around the Go API if you wanted to avoid shelling out, but I am guessing the binary might be the easiest option

@est31
Copy link
Member

est31 commented May 23, 2020

Good idea with the binary. I've filed an issue: #34.

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