AFAIK, orders in chains are not guaranteed, thus this feels applicable to your use case only.

if a chain contains no intermediates, this line will panic.

AFAIK, orders in chains are not guaranteed, thus this feels applicable to your use case only.

I can't find anything explicit in RFC 5280 or RFC 7468 to say that the leaf certificate must be first. However, I can see that the crypto/tls library makes this assumption in its certificate verification logic as well an explicit comment elsewhere that the leaf certificate should be first.

Since I can't find an explicit statement in the standards I'm going to have to fall back on the argument that if it's good enough for Golang's standard TLS implementation then it's good enough for the certificate verification helpers in kubeadm. The Kubernetes components will be using the TLS implementation anyway, so it makes sense that the kubeadm phases which perform certificate verification behave the same way.

if a chain contains no intermediates, this line will panic.

It shouldn't panic, if there's only a single certificate then intermediates will be an empty slice. Here's a quick example:

package main

import "fmt"

func main() {
    x := []int{1}
    fmt.Printf("Head: %d\n", x[0])
    fmt.Printf("Tail: %v\n", x[1:])
}

This produces:

Head: 1
Tail: []

RFC5286 7.4.2 describes the chain order:

The sender's certificate MUST come first in the list. Each following certificate MUST directly certify the one preceding it. Because certificate validation requires that root keys be distributed independently, the self-signed certificate that specifies the root certificate authority MAY be omitted from the chain, under the assumption that the remote end must already possess it in order to validate it in any case.

It shouldn't panic,

my mistake.

RFC5286 7.4.2 describes the chain order:

thanks for the info!

this bool should be the last arg ideally.

Change made in this commit: 6f90532

this t.Errorf does not need to span on multiple lines.

(EDIT: applies to a number of instances bellow too)

All requested formatting changes have been made in this commit: d43e524

The original style that I went with is more consistent with the rest of the existing code (see this example) but I also prefer the shortened version as long as we're not going for complete consistency.

ditto

we should exclude whitespace changes from the PR.

this change doesn't seem backwards compatible to clusters created with older versions of kubeadm.
calling VerifyCertChain would now result in an error if the intermediate pool is empty, correct?

It is backwards compatible since the Verify function doesn't require the intermediate pool to contain any certificates, it only needs to contain the certificates required to construct a valid chain. No additional certificates are required when the leaf certificate is signed directly by the root CA, so this function will succeed.

I've also only changed the verification logic and haven't changed anything to do with the way that kubeadm generates certificates, so the standard setup will still be where the root CA directly signs the leaf certificate.

I've added an extra test case to prove that this is backwards compatible: 511f498

Yup, this should work fine AFAIK.

config should be the first argument.

maybe the better API here is to have a single NewCertificateAuthority function that decides on self-signed vs signed-by-parent depending on a nil parentCert, but having a separate function is fine too.

Since creating an intermediate CA is an uncommon use case (it's only used in the tests for the certificate chain verification) I think it's preferable not to force callers of NewCertificateAuthority (of which there are quite a few) to have to specify a nil certificate and key.

Change made in this commit:

latest commit still has config as the last function argument.

should be:

NewIntermediateCertificateAuthority(config *CertConfig, parentCert *x509.Certificate, parentKey crypto.Signer)...

Sorry, the comment about the change being made was intended for the isCA bool being moved to be the last argument. I've edited the comments so that it makes sense now.

As for moving config to the last argument, I've gone with this ordering for consistency with th existing function NewCertAndKey. Let me know if you would prefer it to be left as is for consistency, be different or whether I should update both to move config to the last argument.

ok, let's leave the ordering as is, for consistency with NewCertAndKey.
the pkiutils package has no API guaranties yet (public usage), but we may have to review some of these semantics at some point.

i wonder if we should just remove the mandatory len(cfg.Usages) > 0 here?
i believe currently we are passing either x509.ExtKeyUsageClientAuth or x509.ExtKeyUsageServerAuth to this function via config.

The config is passed through from NewCertAndKey so it's probably cleaner if I just perform this check there instead.

Let me know if you prefer the version in this commit otherwise I'm happy to remove it entirely: 464418c

The config is passed through from NewCertAndKey so it's probably cleaner if I just perform this check there instead.

agreed.

https://golang.org/pkg/crypto/x509/

// BasicConstraintsValid indicates whether IsCA, MaxPathLen,
// and MaxPathLenZero are valid.

the documentation is a bit vague on what "valid" means, but should we assume BasicConstraintsValid should be always bound to isCA?

e.g. here it's not:

can we pass true always to it and an explicit value to MaxPathLen and MaxPathLenZero=false?

i'm assuming this is where it's checked:
https://github.com/golang/go/blob/master/src/crypto/x509/verify.go#L721-L726

I just did it this way to make sure I didn't make any changes to the existing code which signs leaf certificates. My interpretation of the docs is that it would probably work to set BasicConstraintsValid to true. We shouldn't need to explicitly set MaxPathLen and MaxPathLenZero since the default values of 0 and false respectively are "interpreted as MaxPathLen not being set" according the docs.

If you're happy for this change to be made to the leaf certificate signing then it's in this commit: 081104c

If you want to play it safe then I can revert the commit.

my vote goes to set it always to true even for non-CA certificates.
BasicConstraintsValid seems like a Golang wrapper over the Basic Constraints extension, to conform checks in crypto/x509/verify.go and it doesn't seem like a breaking change.