Validate user provided certificates

[petrutlucian94]

We'll validate user provided certificates, checking fields such as:

• Common Name
• Organization
• NotBefore, NotAfter
• DNS Subject Alternative Name

Also, we ensure that the certificates are signed by the specified CAs.

[bschimke95]

LGTM, only minor nits and a question

[bschimke95]

Is this IPv6-safe?
I guess ip cannot be [<ipv6>] so that we would have double brackets, right?

[petrutlucian94]

Good point. The ip address wouldn't include a port, so no need for square brackets. I haven't checked how VerifyHostname handles double brackets, we could just do something like strings.Trim(ip, "[]").

However, we aren't using this check right now so we might as well remove it.

[HomayoonAlimohammadi]

Great work! Thanks a lot @petrutlucian94. Left some comments mostly for discussion. Feel free to ignore them if they don't seem relevant.

[HomayoonAlimohammadi]

I think it's better to have functionalities like time, passed like this to make it more testable:

type CertValidator struct {
    nowFn func() time.Time
    ...
}

NewCertValidator(time.Now, ...)

[petrutlucian94]

IMO this seems unnecessary and would complicate the caller code. We already have tests that check the NotBefore and NotAfter fields without having to pass a time function.

[HomayoonAlimohammadi]

I would personally go with a dependency injection and interface. Something like:

type CertValidator interface {
    Validate(certPEM, certKey string, opts... certvalidator.Option) error
}

type ControlPlanePKI struct {
    certValidator CertValidator
    ...
}

func NewControlPlanePKI(certValidator CertValidator, ...) *ControlPlanePKI {
    ...
    return &ControlPlanePKI{
        certValidator: certValidator,
        ...
    }
}

func (c *ControlPlanePKI) CompleteCertificates() error {
    ...
    if err := c.certValidator.Validate(certPEM, certKey, certvalidator.WithAllowSelfSigned(), certvalidator.WithCanonicalName(cn)) {
        return fmt.Errorf("failed to validate certificate: %w")
    }
    ...
}
}

and in utils/pki/certvalidator/validator.go:

package certvalidator

type Validator struct {
    nowFn func() time.Time
    ...
}

type Option func(v *Validator)

func WithCanonicalName(name string) Option {
    return func(v *Validator) {
        v.cn = name
   }
}

func (v *Validator) Validate(certPEM, certKey string, options ...Option) error {
    for _, o in range options {
        o(v)
    }

    ...
}

I think it's more testable this way but I don't have any strong objections with the current implementation.

[petrutlucian94]

Thanks for the suggestion but again I think this would complicate the implementation unnecessarily. We have just one concrete implementation and we don't need to stub it for testing purposes, in which case I don't see the point of using an interface and dependency injection.