-
Notifications
You must be signed in to change notification settings - Fork 3
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
X509lax #157
base: main
Are you sure you want to change the base?
X509lax #157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool, I like the direction - huge thanks for putting the substantial effort in here to drop the dep on the old fork!
We should probably be explicit in the pkg doc about the intention here for future folks coming along - e.g.
- not a full validating x509 impl for TLS use, but bare bones for running a CT log; should not be reused elsewhere for safety reasons.
- Not likely to be frequently/if at all updated from the upstream stdlib x509
etc.
internal/x509util/verify.go
Outdated
} | ||
} | ||
// CANotAuthorizedForThisName check deleted. | ||
// Allow to log all certificates, even if they have been isued by a CA that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "Allow logging of all ..."
internal/x509util/verify.go
Outdated
} | ||
} | ||
// CANotAuthorizedForThisName check deleted. | ||
// Allow to log all certificates, even if they have been isued by a CA that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isued/issued
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the verify.go
was deleted in the next commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next PR will start to use verify.go
, rather than using verify.go
from the crypto/x509
library. Otherwise, the crypto/x509
library would not verify chains submitted to CT logs.
internal/x509util/verify.go
Outdated
} | ||
// CANotAuthorizedForThisName check deleted. | ||
// Allow to log all certificates, even if they have been isued by a CA that | ||
// is not auhotized to issue certs for a given domain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/auhotized/authorized/ :)
internal/x509util/x509.go
Outdated
package x509util | ||
|
||
var ( | ||
oidExtensionSubjectAltName = []int{2, 5, 29, 17} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is going to be used, please add the doc comment to add the OID description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's currently used by verify.go
.
I could put a description indeed, but it would literally be the variable name..."oid for Subject Alternative Name extension"
Note that the crypto/x509 library doesn't have a description, neither does our c-t-go fork.
It's the only thing we need from x509.go
.. I could put it in a different file, I've decided to leave it there to stay as close as possible to upstream.
internal/x509util/x509.go
Outdated
package x509util | ||
|
||
var ( | ||
oidExtensionSubjectAltName = []int{2, 5, 29, 17} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is going to be used, please add the doc comment to add the OID description.
…-dev#139) * Copy client code from transparency-dev/trillian-tessera * Add testdata for client tests # Conflicts: # internal/client/client.go # internal/client/fetcher.go
* Add CT Hammer * Update CT Hammer README.md * Remove TODOs in `EntryBundle.UnmarshalText` func * Update `EntryBundle.UnmarshalText` func doc comment * Verify supported key algorithm after loading the private keys # Conflicts: # internal/hammer/README.md # internal/hammer/hammer.go # internal/hammer/loadtest/client.go
Bumps the all-deps group with 5 updates: | Package | From | To | | --- | --- | --- | | [cloud.google.com/go/spanner](https://github.com/googleapis/google-cloud-go) | `1.75.0` | `1.76.1` | | [github.com/google/go-cmp](https://github.com/google/go-cmp) | `0.6.0` | `0.7.0` | | [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang) | `1.20.5` | `1.21.0` | | [golang.org/x/crypto](https://github.com/golang/crypto) | `0.33.0` | `0.34.0` | | [google.golang.org/api](https://github.com/googleapis/google-api-go-client) | `0.221.0` | `0.222.0` | Updates `cloud.google.com/go/spanner` from 1.75.0 to 1.76.1 - [Release notes](https://github.com/googleapis/google-cloud-go/releases) - [Changelog](https://github.com/googleapis/google-cloud-go/blob/main/CHANGES.md) - [Commits](googleapis/google-cloud-go@spanner/v1.75.0...spanner/v1.76.1) Updates `github.com/google/go-cmp` from 0.6.0 to 0.7.0 - [Release notes](https://github.com/google/go-cmp/releases) - [Commits](google/go-cmp@v0.6.0...v0.7.0) Updates `github.com/prometheus/client_golang` from 1.20.5 to 1.21.0 - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/main/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.20.5...v1.21.0) Updates `golang.org/x/crypto` from 0.33.0 to 0.34.0 - [Commits](golang/crypto@v0.33.0...v0.34.0) Updates `google.golang.org/api` from 0.221.0 to 0.222.0 - [Release notes](https://github.com/googleapis/google-api-go-client/releases) - [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md) - [Commits](googleapis/google-api-go-client@v0.221.0...v0.222.0) --- updated-dependencies: - dependency-name: cloud.google.com/go/spanner dependency-type: direct:production update-type: version-update:semver-minor dependency-group: all-deps - dependency-name: github.com/google/go-cmp dependency-type: direct:production update-type: version-update:semver-minor dependency-group: all-deps - dependency-name: github.com/prometheus/client_golang dependency-type: direct:production update-type: version-update:semver-minor dependency-group: all-deps - dependency-name: golang.org/x/crypto dependency-type: direct:production update-type: version-update:semver-minor dependency-group: all-deps - dependency-name: google.golang.org/api dependency-type: direct:production update-type: version-update:semver-minor dependency-group: all-deps ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Toward #120.
This PR copies the smallest amount of code we can, allowing to build Tesseract without a dep on c-t-go/x509, and using "crypto/x509" wherever it can.
It is WIP, but ready for someone to take a first pass, and confirm that this PR can easily be reviewed.
A followup PR will migrate the codebase to this fork + "crypto/x509". You can have a sneak peek here: https://github.com/phbnf/static-ct/tree/evenmoreold.
This PR has two commits. To review this PR, review commits independtly.
verify.go
andcert_pool.go
from "crypto/x509" Not much to review there, it's a copy.- changes package name
- takes a dep on "crypto/x509" whenever we can
(c *Certificate) func(args)
tofunc(c x509.Certificate, args)
to re-use thex509.Certificate
object without redefining itCertificateInvalidError{c} --> CertificateInvalidError{Cert: c}
x509.go
. I've left it in a smallx509.go
file just to avoid extra modifications.TODO(phboneff):