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

Extending SANs DNS entries #170

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Extending SANs DNS entries #170

wants to merge 9 commits into from

Conversation

ctyano
Copy link
Collaborator

@ctyano ctyano commented Nov 14, 2024

Description

This Pull Requests adds capability to set additional Subjct Alternative Names DNS field to the identity certificates issued by SIA.
Currently, the implementation restricts to have only certain syntaxes in SANs DNS and this restriction is limiting the use of the identity certificates for server certificate purposes.
This enhancement will bring more flexibility to add SANs DNS that matches their actual DNS domains.
For the security perspective it is recommended to always verify the SANs DNS in CSR by the instance identity provider.

Assignees

  • Assignees is set

Type of changes

  • Apply one or more labels of the following that fits:
    • enhancement: New Feature

Flags

  • Breaks backward compatibility
  • Requires a documentation update
  • Has untestable code

Checklist

  • Followed the guidelines in the CONTRIBUTING document
  • Added prefix [skip ci]/[ci skip]/[no ci]/[skip actions]/[actions skip] in the PR title if necessary
  • Tested and linted the code
  • Commented the code
  • Made corresponding changes to the documentation

Checklist for maintainer

  • Use Squash and merge
  • Double-confirm the merge message has prefix [skip ci]/[ci skip]/[no ci]/[skip actions]/[actions skip]
  • Delete the branch after merge

@mlajkim
Copy link
Contributor

mlajkim commented Nov 15, 2024

@mlajkim mlajkim added the enhancement New feature or request label Nov 19, 2024
@mlajkim
Copy link
Contributor

mlajkim commented Nov 19, 2024

  • Please write the description in a simple manner

pkg/certificate/identity.go Outdated Show resolved Hide resolved
athenz-sia.env Outdated Show resolved Hide resolved
pkg/config/derived-service-cert.go Outdated Show resolved Hide resolved
pkg/config/derived-service-cert.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mlajkim mlajkim left a comment

Choose a reason for hiding this comment

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

simple review.

@@ -48,6 +49,7 @@ func (idCfg *IdentityConfig) derivedServiceCertConfig() error {
Provider: "",
AthenzDomainName: "",
AthenzServiceName: "",
CertExtraSANDNSs: []string{},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should change in a way so that you can set the following in pkg/certificate/identity.go:

SANs: util.SubjectAlternateNames{
	DNSNames: idCfg.ServiceCert.CopperArgos.Sans,
	URIs:     []url.URL{*spiffeURI},
},

introducing CertExtraSANDNSs under CopperArgosMode doesn't look clean to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Made a sample PR: #179

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mlajkim To simplify the PR, could you make the PR against ctyano:tatyano, so that we can include the both of this original PR and your proposal?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants