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

Add NewCertificateFromX509 function #239

Closed
wants to merge 5 commits into from

Conversation

hslatman
Copy link
Member

@hslatman hslatman commented May 18, 2023

The new function can create a x509util.Certificate from an x509.Certificate (used as a template). It also supports options, to provide a template, so that data can be set in the certificate.

Currently a template is always required. An error will be returned when no template is provided.

@maraino: I'll add tests later if this approach seems sensible to you. I don't think we can use generics to handle both x509.Certificate and x509.CertificateRequest options using only a single implementation, so I copied them over.

The new function can create a `x509util.Certificate` from an
x509.Certificate (used as a template). It also supports options,
to provide a template, so that data can be set in the certificate.

Currently a template is always required. An error will be returned
when no template is provided.
@maraino
Copy link
Contributor

maraino commented May 19, 2023

I ❤️ the option of using generics.

@hslatman
Copy link
Member Author

hslatman commented May 25, 2023

@maraino I made the options generic in 5199601. Haven't updated all usages yet to keep the diff readable. Does this look reasonable to you? We'll need to update usages of Options in dependent code. Should be fairly quick to do.

If the approach looks OK, I'll add some tests.

@hslatman
Copy link
Member Author

hslatman commented May 25, 2023

@maraino I've gone ahead and updated the usages within crypto.

Do you think it's useful to provide helper functions like these:

func WithCSRTemplate(text string, data TemplateData) Option[*x509.CertificateRequest] {
	return WithTemplate[*x509.CertificateRequest](text, data)
}
// or:
func WithCertificateRequestTemplate(text string, data TemplateData) Option[*x509.CertificateRequest] {
	return WithTemplate[*x509.CertificateRequest](text, data)
}

Usage would then look a bit cleaner:

fn := WithCSRTemplate(tt.args.text, tt.args.data)

Instead of

fn := WithTemplate[*x509.CertificateRequest](tt.args.text, tt.args.data)

Defining aliases like the below is possible, but then requires defining the apply too:

type CertificateOptions Options[*x509.Certificate]

func (co *CertificateOptions) apply(t *x509.Certificate, opts []Option[*x509.Certificate]) (*Options[*x509.Certificate], error) {
	o := &Options[*x509.Certificate]{}
	return o.apply(t, opts)
}

Then another thing we could add is a type alias for a single CertificateOption (and CertificateRequestOption):

type CertificateOption Option[*x509.Certificate]

func (co *CertificateOptions) apply(t *x509.Certificate, opts []CertificateOption) (*Options[*x509.Certificate], error) {
	o := &Options[*x509.Certificate]{}
	var copts []Option[*x509.Certificate]
	for _, opt := range opts {
		copts = append(copts, Option[*x509.Certificate](opt))
	}
	return o.apply(t, copts)
}

// resulting in this function signature:
func NewCertificateFromX509(template *x509.Certificate, opts ...CertificateOption) (*Certificate, error)

Let me know if you think the above things would be useful, and which you would like to have available.

@hslatman
Copy link
Member Author

Closing in favor of #248.

@hslatman hslatman closed this May 26, 2023
@hslatman hslatman removed the request for review from maraino May 26, 2023 13:46
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.

2 participants