LiteralSubject: Add support for numeric OID subject attribute type

[inteon]

This PR adds a missing feature to the UnmarshalSubjectStringToRDNSequence function: the ability to specify numeric OIDs as subject attribute type. Right now, an attribute type has to be chosen by name and only a small set of names is supported:

cert-manager/pkg/util/pki/subject.go

Lines 55 to 66 in 8d15b5e

	var attributeTypeNames = map[string][]int{
	"C": OIDConstants.Country,
	"O": OIDConstants.Organization,
	"OU": OIDConstants.OrganizationalUnit,
	"CN": OIDConstants.CommonName,
	"SERIALNUMBER": OIDConstants.SerialNumber,
	"L": OIDConstants.Locality,
	"ST": OIDConstants.Province,
	"STREET": OIDConstants.StreetAddress,
	"DC": OIDConstants.DomainComponent,
	"UID": OIDConstants.UniqueIdentifier,
	}

Kind

/kind feature

Release Note

Add support for numeric OID types in LiteralSubject. Eg. "1.2.3.4=String Value"

[jetstack-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from inteon. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jetstack-bot]

@inteon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cert-manager-master-make-test	53aefb8	link	true	/test pull-cert-manager-master-make-test
pull-cert-manager-master-e2e-v1-28	53aefb8	link	true	/test pull-cert-manager-master-e2e-v1-28

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[inteon]

/test pull-cert-manager-master-make-verify

[inteon]

/retest

[wallrj]

I think this should be broken in to separate PRs for some further explanation and discussion:

1. Revert removal of the openldap library. I'm really pleased to see the improvements that you've pushed upstream to openldap, and I'm pleased that we won't have to maintain our own subject parsing code. But previously you'd argued that the library was a liability; too large and too flawed for the sake of a single ParseDN function. Have you changed your mind?

2. Remove deprecated function: I think all deprecations and removals should have a separate release note.

3. Add fuzz and roundtrip tests and rename test file: I expected that the fuzz tests would obsolete some of the existing tests, and that those could be removed. In a separate PR explain with examples why the fuzz tests are required and why the original tests are still required.

4. New feature: the ability to specify literal OIDs as subject attribute type. This should have it's own PR with a release note and a real-world example Certificate in the PR description to demonstrate the use-case. It should probably have a website PR too, to update the literalSubject docs with an example.

[inteon]

1. Revert removal of the openldap library. I'm really pleased to see the improvements that you've pushed upstream to openldap, and I'm pleased that we won't have to maintain our own subject parsing code. But previously you'd argued that the library was a liability; too large and too flawed for the sake of a single ParseDN function. Have you changed your mind?

I still think it is overkill, but I agree with you that it is non-ideal to maintain a separate version. Also, the openssf score is 5.8, which is not crazy bad: https://cert-manager.io/docs/announcements/AdaLogics-2023-cert-manager-audit-report.pdf.

I'll create some branched-out PRs.

[inteon]

@wallrj All branched-out PRs have been merged. Now, only the literal OID type support remains in this PR.

[wallrj]

A couple of questions about the changes.
And I suggest putting a real-world example in the release note.
Is there a real-world example?
Perhaps this:

$ curl -fsSL https://raw.githubusercontent.com/pyca/cryptography/main/vectors/cryptography_vectors/x509/belgian-eid-invalid-visiblestring.pem | step certificate inspect
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 21267647932559206558027296640314674717 (0x10000000000075040c80ae5b453c8e1d)
    Signature Algorithm: SHA256-RSA
        Issuer: C=BE,CN=Citizen CA,SERIALNUMBER=201623
        Validity
            Not Before: Aug 29 09:47:00 2016 UTC
            Not After : Aug 24 23:59:59 2026 UTC
        Subject: C=BE,CN=Else De Proft (Signature),UnknownOID=2.5.4.4,UnknownOID=2.5.4.42,SERIALNUMBER=69070338850

Has someone requested this in a GitHub issue?
If so, link to it.

The RFC doesn't talk about "literal" OID. It talks about "numeric" OID versus "named" OID, so I suggest using "numeric" OID in the PR title, description, and code.

[wallrj]

What's the significance of nil vs []int{}?
Isn't it going to be nil anyway (assuming ParseObjectIdentifier returns nil, err)?

[inteon]

Indeed, I think ParseObjectIdentifier returns nil, err.
I set oid = nil here to accentuate that we actually rely on this oid value being nil.
In

cert-manager/internal/apis/certmanager/validation/certificate.go

Lines 95 to 102 in 1035bb4

	// Should not contain unrecognized OIDs
	for _, rdns := range sequence {
	for _, atv := range rdns {
	if atv.Type.Equal(nil) {
	el = append(el, field.Invalid(fldPath.Child("literalSubject"), crt.LiteralSubject, fmt.Sprintf("Literal subject contains unrecognized key with value [%s]", atv.Value)))
	}
	}
	}

we actually rely on this Type == nil setup.

[wallrj]

Great.

/lgtm
/approve

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [wallrj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment