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.

Sign up for GitHub

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

Jump to bottom

Add attribute support for certificate subject #129

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add attribute support for certificate subject #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion pkg/apis/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,17 @@ const (
IssuerKindKey = "csi.cert-manager.io/issuer-kind"
IssuerGroupKey = "csi.cert-manager.io/issuer-group"

CommonNameKey = "csi.cert-manager.io/common-name"
LiteralSubjectKey = "csi.cert-manager.io/literal-subject"
CommonNameKey = "csi.cert-manager.io/common-name"
OrganizationsKey = "csi.cert-manager.io/organizations"
OrganizationalUnitsKey = "csi.cert-manager.io/organizationalunits"
CountriesKey = "csi.cert-manager.io/countries"
ProvincesKey = "csi.cert-manager.io/provinces"
LocalitiesKey = "csi.cert-manager.io/localities"
StreetAddressesKey = "csi.cert-manager.io/streetaddresses"
PostalCodesKey = "csi.cert-manager.io/postalcodes"
SerialNumberKey = "csi.cert-manager.io/serialnumber"

DNSNamesKey = "csi.cert-manager.io/dns-names"
IPSANsKey = "csi.cert-manager.io/ip-sans"
URISANsKey = "csi.cert-manager.io/uri-sans"
Expand Down
14 changes: 14 additions & 0 deletions pkg/apis/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ func Test_ValidateAttributes(t *testing.T) {
expError error
}

var literalSubject = "CN=${POD_NAME}.${POD_NAMESPACE}.svc.cluster.local,OU=0:${POD_NAME}\\;1:${POD_NAMESPACE}\\;2:my-region\\;4:unittest,O=foo.bar.com"

tests := map[string]struct {
attr map[string]string
expErr field.ErrorList
Expand Down Expand Up @@ -206,6 +208,18 @@ func Test_ValidateAttributes(t *testing.T) {
field.Invalid(field.NewPath("volumeAttributes", "csi.cert-manager.io/privatekey-file"), "/foobar", "filename must not include '/'"),
},
},
"correct literal-subject should not error": {
attr: map[string]string{
csiapi.IssuerNameKey: "test-issuer",
csiapi.LiteralSubjectKey: literalSubject,
csiapi.CAFileKey: "ca.crt",
csiapi.CertFileKey: "crt.tls",
csiapi.KeyFileKey: "key.tls",
csiapi.DNSNamesKey: "foo.bar.com",
csiapi.KeyEncodingKey: "PKCS8",
},
expErr: nil,
Comment on lines +211 to +221
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: this will need many more test cases IMO, to ensure that the splitting functionality works as expected!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SgtCoDFish If I may push back, I felt like if I added too many permutations, I'd essentially be testing github.com/cert-manager/cert-manager/pkg/util/pki.ParseSubjectStringToRawDerBytes, because that's doing the heavy lifting in this specific case. That said, if you think there is a branch of this logic that could use a better test, I'm open to anything.

},
}

for name, test := range tests {
Expand Down
60 changes: 44 additions & 16 deletions pkg/requestgen/generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import (

cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
cmpki "github.com/cert-manager/cert-manager/pkg/util/pki"

"github.com/cert-manager/csi-lib/manager"
"github.com/cert-manager/csi-lib/metadata"

Expand Down Expand Up @@ -56,32 +58,58 @@ func RequestForMetadata(meta metadata.Metadata) (*manager.CertificateRequestBund
}
}

commonName, err := expand(meta, attrs[csiapi.CommonNameKey])
if err != nil {
return nil, fmt.Errorf("%q: %w", csiapi.CommonNameKey, err)
var request = &x509.CertificateRequest{}
if lSubjStr, ok := attrs[csiapi.LiteralSubjectKey]; ok && len(lSubjStr) > 0 {
lSubjStr, err = expand(meta, lSubjStr)
if err != nil {
return nil, fmt.Errorf("%q: %w", csiapi.LiteralSubjectKey, err)
}
request.RawSubject, err = cmpki.ParseSubjectStringToRawDerBytes(lSubjStr)
if err != nil {
return nil, fmt.Errorf("%q: %w", csiapi.LiteralSubjectKey, err)
}
} else {
request.Subject = pkix.Name{}
request.Subject.CommonName, err = expand(meta, attrs[csiapi.CommonNameKey])
if err != nil {
return nil, fmt.Errorf("%q: %w", csiapi.CommonNameKey, err)
}
if len(attrs[csiapi.SerialNumberKey]) > 0 {
request.Subject.SerialNumber = attrs[csiapi.SerialNumberKey]
}
for k, v := range map[*[]string]string{
&request.Subject.Organization: csiapi.OrganizationsKey,
&request.Subject.OrganizationalUnit: csiapi.OrganizationalUnitsKey,
&request.Subject.Country: csiapi.CountriesKey,
&request.Subject.Province: csiapi.ProvincesKey,
&request.Subject.Locality: csiapi.LocalitiesKey,
&request.Subject.StreetAddress: csiapi.StreetAddressesKey,
&request.Subject.PostalCode: csiapi.PostalCodesKey,
} {
if len(attrs[v]) > 0 {
var e, err = expand(meta, attrs[v])
if err != nil {
return nil, fmt.Errorf("%q: %w", v, err)
}
*k = strings.Split(e, ",")
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: strings.Split might not be suitable here, since several of these fields can reasonably contain commas.

I remember that you commented on a PR implementing a similar feature in cert-manager: cert-manager/cert-manager#4502 (comment)

I think we're likely to need a similar approach here as I suggested in there. CSV parsing seems like a good solution.

Example test string:

`"1725 Slough Avenue, Suite 200, Scranton Business Park","10 Downing Street, Westminster",Something Else`

Should produce exactly 3 street address entries

Copy link
Member

Choose a reason for hiding this comment

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

@cornfeedhobo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@inteon Hey, I unfortunately don't have the time to revisit this and my team ended up scraping the associated project. Anyone is welcome to pick up where I left off, otherwise it will probably be a few months until I can carve out time.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for letting us know!

}
}
}
dns, err := parseDNSNames(meta, attrs[csiapi.DNSNamesKey])
request.DNSNames, err = parseDNSNames(meta, attrs[csiapi.DNSNamesKey])
if err != nil {
return nil, fmt.Errorf("%q: %w", csiapi.DNSNamesKey, err)
}
uris, err := parseURIs(meta, attrs[csiapi.URISANsKey])
request.IPAddresses, err = parseIPAddresses(attrs[csiapi.IPSANsKey])
if err != nil {
return nil, fmt.Errorf("%q: %w", csiapi.URISANsKey, err)
return nil, fmt.Errorf("%q: %w", csiapi.IPSANsKey, err)
}
ips, err := parseIPAddresses(attrs[csiapi.IPSANsKey])
request.URIs, err = parseURIs(meta, attrs[csiapi.URISANsKey])
if err != nil {
return nil, fmt.Errorf("%q: %w", csiapi.IPSANsKey, err)
return nil, fmt.Errorf("%q: %w", csiapi.URISANsKey, err)
}

return &manager.CertificateRequestBundle{
Request: &x509.CertificateRequest{
Subject: pkix.Name{
CommonName: commonName,
},
DNSNames: dns,
IPAddresses: ips,
URIs: uris,
},
Request: request,
IsCA: strings.ToLower(attrs[csiapi.IsCAKey]) == "true",
Namespace: attrs[csiapi.K8sVolumeContextKeyPodNamespace],
Duration: duration,
Expand Down
35 changes: 35 additions & 0 deletions pkg/requestgen/generator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"errors"
"net"
"net/url"
"strings"
"testing"
"time"

Expand All @@ -31,6 +32,9 @@ import (

cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
cmpki "github.com/cert-manager/cert-manager/pkg/util/pki"

csiapi "github.com/cert-manager/csi-driver/pkg/apis/v1alpha1"
)

func Test_RequestForMetadata(t *testing.T) {
Expand All @@ -52,6 +56,12 @@ func Test_RequestForMetadata(t *testing.T) {
return puri
}

var literalSubject = "CN=my-pod.my-namespace.svc.cluster.local,OU=0:my-pod\\;1:my-namespace\\;2:my-region\\;4:unittest,O=foo.bar.com"
var rawLiteralSubject, err = cmpki.ParseSubjectStringToRawDerBytes(literalSubject)
if err != nil {
assert.NoError(t, err)
}

tests := map[string]struct {
meta metadata.Metadata
expRequest *manager.CertificateRequestBundle
Expand Down Expand Up @@ -174,6 +184,31 @@ func Test_RequestForMetadata(t *testing.T) {
},
expErr: false,
},
"a metadata with literal subject set should be returned": {
meta: baseMetadataWith(metadata.Metadata{VolumeContext: map[string]string{
csiapi.IssuerNameKey: "my-issuer",
csiapi.LiteralSubjectKey: literalSubject,
}}),
expRequest: &manager.CertificateRequestBundle{
Request: &x509.CertificateRequest{RawSubject: rawLiteralSubject},
Usages: cmapi.DefaultKeyUsages(),
Namespace: "my-namespace",
IssuerRef: cmmeta.ObjectReference{
Name: "my-issuer",
Kind: "Issuer",
Group: "cert-manager.io",
},
Duration: cmapi.DefaultCertificateDuration,
},
expErr: false,
},
"a metadata with incorrect literal subject set should error": {
meta: baseMetadataWith(metadata.Metadata{VolumeContext: map[string]string{
csiapi.IssuerNameKey: "my-issuer",
csiapi.LiteralSubjectKey: strings.Replace(literalSubject, ";", "&", -1),
}}),
expErr: true,
},
}

for name, test := range tests {
Expand Down