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

X509lax #157

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

X509lax #157

wants to merge 2 commits into from

Conversation

phbnf
Copy link
Collaborator

@phbnf phbnf commented Feb 25, 2025

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.

  • one that copies verify.go and cert_pool.go from "crypto/x509" Not much to review there, it's a copy.
  • one that removes all the things we either don't need to run a CT log, or would break a CT log. Here are all the modification it brings:
    • Deletes checks that would not pass with certificates submitted to CT logs, such as criticalExtensions checks, because pre-certs have a critical pre cert extension that the crypto/x509 library does not recognize
      - changes package name
      - takes a dep on "crypto/x509" whenever we can
    • modify some method signatures from (c *Certificate) func(args) to func(c x509.Certificate, args) to re-use the x509.Certificate object without redefining it
    • sets struct field names in declaration when not set such as: CertificateInvalidError{c} --> CertificateInvalidError{Cert: c}
    • copies over a very small oid definition from x509.go. I've left it in a small x509.go file just to avoid extra modifications.

TODO(phboneff):

  • update to go 1.24, and use NoValidChains
  • remove some TODOs
  • add Readme that explains what this package is, and why it's there

Copy link
Collaborator

@AlCutter AlCutter left a 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.

}
}
// CANotAuthorizedForThisName check deleted.
// Allow to log all certificates, even if they have been isued by a CA that
Copy link
Collaborator

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 ..."

}
}
// CANotAuthorizedForThisName check deleted.
// Allow to log all certificates, even if they have been isued by a CA that
Copy link
Collaborator

Choose a reason for hiding this comment

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

isued/issued

Copy link
Contributor

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.

}
// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

/auhotized/authorized/ :)

package x509util

var (
oidExtensionSubjectAltName = []int{2, 5, 29, 17}
Copy link
Collaborator

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?

Copy link
Contributor

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.

package x509util

var (
oidExtensionSubjectAltName = []int{2, 5, 29, 17}
Copy link
Contributor

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.

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.

3 participants