Skip to content

Other name sans support in Certificates#6404

Merged
jetstack-bot merged 22 commits intocert-manager:masterfrom
SpectralHiss:hef/otherNameSANs
Jan 3, 2024
Merged

Other name sans support in Certificates#6404
jetstack-bot merged 22 commits intocert-manager:masterfrom
SpectralHiss:hef/otherNameSANs

Conversation

@SpectralHiss
Copy link
Copy Markdown
Contributor

@SpectralHiss SpectralHiss commented Oct 10, 2023 •
edited
Loading

Pull Request Motivation

Addresses #6393
Adds OtherNameSANs field to Certificates
* Added an otherName SAN extension mechanism that allows for otherName(s) to be set.
* Can take any otherName OID with String (UTF-8) like value
* cf RFC 5280 p 37
* otherName is only a subset of GeneralName, our specific need for for
UserPrincipalName used in Microsoft AD/ LDAP is met but we can do any string otherName with this feature.
* shallow validation logic in supplied oid type
* Ensured that the selfsigned CertificateTemplateFromCSR still works fine in test/e2e/certificates/othernamesan.go

Kind

feature

Release Note

Added support for `otherName` SANS in Certificates

@jetstack-bot jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 10, 2023
@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Oct 10, 2023

Hi @SpectralHiss. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration labels Oct 10, 2023
inteon
inteon reviewed Oct 10, 2023
View reviewed changes
inteon
inteon reviewed Oct 10, 2023
View reviewed changes
inteon
inteon reviewed Oct 10, 2023
View reviewed changes
inteon
inteon reviewed Oct 10, 2023
View reviewed changes
@inteon
Copy link
Copy Markdown
Member

inteon commented Oct 10, 2023

/ok-to-test

@jetstack-bot jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 10, 2023
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Oct 11, 2023
@SpectralHiss SpectralHiss changed the title WIP: Hef/other name sans Other name sans support in Certificates Oct 11, 2023
@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 11, 2023
@SpectralHiss
Copy link
Copy Markdown
Contributor Author

SpectralHiss commented Oct 11, 2023

flake..
/retest-required

inteon
inteon reviewed Oct 11, 2023
View reviewed changes
@inteon inteon self-assigned this Oct 11, 2023
inteon
inteon reviewed Oct 11, 2023
View reviewed changes
inteon
inteon reviewed Oct 11, 2023
View reviewed changes
SpectralHiss added 3 commits January 2, 2024 08:47
The sample issuer won't work with OtherName CSR
7b96701 
* The sample code leverages standard library only
* It does not leverage util/pki from cert-manager nor issuer-lib

Signed-off-by: SpectralHiss <houssem.elfekih@jetstack.io>
Allow other SANS in Vault e2e framework
7f349ef 
* This is to enable conformance testing of the otherName alpha feature

Signed-off-by: SpectralHiss <houssem.elfekih@jetstack.io>
Add order agnostic matcher for SANs
7350863 
* This is to ensure Vault conformance passes since it outputs SANs in
  different order to other issuers
* Matcher was tested manually only we will add tests to it in future

Signed-off-by: SpectralHiss <houssem.elfekih@jetstack.io>
SgtCoDFish
SgtCoDFish reviewed Jan 3, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

Got a few comments - I'm gonna take a break soon because I'm ill so I might not be able to re-review. I'd be fine with merging if the comments are addressed though - someone else could review.

Nothing major here though - I think this is great 😁

@SpectralHiss @SgtCoDFish
Update internal/apis/certmanager/types_certificate.go
5cc5c81 
Co-authored-by: Ashley Davis <SgtCoDFish@users.noreply.github.com>
Signed-off-by: SpectralHiss <houssem.elfekih@jetstack.io>
@SpectralHiss @SgtCoDFish
Update internal/apis/certmanager/types_certificate.go
c90fd33 
Co-authored-by: Ashley Davis <SgtCoDFish@users.noreply.github.com>
Signed-off-by: SpectralHiss <houssem.elfekih@jetstack.io>
@SpectralHiss @SgtCoDFish
Update pkg/util/pki/asn1_util.go
ddc1dff 
Co-authored-by: Ashley Davis <SgtCoDFish@users.noreply.github.com>
Signed-off-by: Houssem El Fekih <hassoum92@hotmail.com>
@inteon
Copy link
Copy Markdown
Member

inteon commented Jan 3, 2024

Thanks @SpectralHiss, this is an awesome new feature.
It took a long time to get this implemented and reviewed, but I think the result is worth it.

/approve
/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2024
@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Jan 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 3, 2024
@jetstack-bot jetstack-bot merged commit cc8925a into cert-manager:master Jan 3, 2024
@wallrj
Copy link
Copy Markdown
Member

wallrj commented Jan 3, 2024

/kind feature

@jetstack-bot jetstack-bot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jan 3, 2024
@wallrj
Copy link
Copy Markdown
Member

wallrj commented Jan 4, 2024

@SpectralHiss Does this need a short paragraph explaining what it can be used for at https://cert-manager.io/docs/usage/certificate/#creating-certificate-resources ?

@SpectralHiss
Copy link
Copy Markdown
Contributor Author

SpectralHiss commented Jan 4, 2024

Yeah, good point! will make a PR to the website documentation

@inteon inteon mentioned this pull request Jan 8, 2024
Closed
@SgtCoDFish
Copy link
Copy Markdown
Member

SgtCoDFish commented Jan 8, 2024

I was off ill when this merged, but I want to chime in now it's merged and say this is absolutely awesome. Great stuff @SpectralHiss 🚀

@SpectralHiss SpectralHiss mentioned this pull request Jan 8, 2024
Merged
@wallrj wallrj mentioned this pull request Jan 17, 2024
Merged
This was referenced Jan 18, 2024
Closed
Closed
@inteon inteon mentioned this pull request Feb 7, 2024
Merged
@maelvls maelvls mentioned this pull request Sep 15, 2025
Merged
@wallrj-cyberark wallrj-cyberark mentioned this pull request Dec 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SgtCoDFish SgtCoDFish SgtCoDFish requested changes

@inteon inteon inteon requested changes

Assignees

@inteon inteon

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration area/testing Issues relating to testing dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. ok-to-test release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

1.14

Development

Successfully merging this pull request may close these issues.

5 participants

@SpectralHiss @jetstack-bot @inteon @wallrj @SgtCoDFish