osbuild: new stage 'cacert'

[lzap]

This pull request adds a new stage called 'cacert' to the osbuild package. It also includes file changes to support the CACustomization feature, which allows users to specify a list of certificates for the CA (Certificate Authority). The changes include updates to the Customizations struct, the osCustomizations function, and the OSCustomizations struct. Additionally, a new function called parseCerts is added to parse the certificate strings. The changes also include updates to the serialize and prependKernelCmdlineStage functions in the OS struct. Finally, a new file called ca_stage.go is added to the osbuild package, which contains the implementation of the NewCAStageStage function.

Needs: osbuild/osbuild#1854

[lzap]

Missing the filename extension, will add during review.

[achilleas-k]

Can we break down this PR into multiple commits for easier reviewing and history?

I like to have commits that introduce new things separate from the following ones that use them, so maybe:

1. Add osbuild/..._stage.go
2. Add manifest/os.go changes.
3. Add blueprint customization.
4. Add config option to all-customizations.json.

(though, 2 and 3 can be merged).

[achilleas-k]

ca, while an accurate initialism and probably a very common term to refer to what we're doing, is probably not a great name for a configuration key in the soup of top-level blueprint customization names.

CertificateAuthority (certificate-authority) is, on the other hand, too long.

Maybe CACerts (toml:"ca-certs") would be a better name?

[lzap]

Sure, in the same struct I see a two-worded JSON field SSHKey as sshkey so I will stick with this theme and go for cacerts.

[achilleas-k]

This doesn't match the customization struct in the code. The way the customization is written now this should be:

"ca": {
  "certs": [
    "..."
  ]
}

[achilleas-k]

The stage is called pki.update-ca-trust so please name the file accordingly for easier navigation.
pki_update_stage.go for example.

[achilleas-k]

Can we break down this PR into multiple commits for easier reviewing and history?

I like to have commits that introduce new things separate from the following ones that use them, so maybe:

1. Add osbuild/..._stage.go
2. Add manifest/os.go changes.
3. Add blueprint customization.
4. Add config option to all-customizations.json.

(though, 2 and 3 can be merged).

[lzap]

Sure, amended all remarks. If you don’t mind, I am keeping one commit for easier work from my side, multiple commits rebasing is a pain to work with this isn’t a big PR by any means. I will split it after the code is ready.

[github-actions]

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

[mvo5]

Thank you for this PR! It looks good but I still have some ideas/suggestions inline for your consideration. But I'm happy to help with them if you have no enough time :)

[mvo5]

Is this the behavior we want/expect? This will mean that pasing an invalid cert string will just result in an empty list and the cert not appearing in the manifest. As a user I would probably be surprised if I paste a cert with an error and then get an image (much later) that does not contain it. Sorry, I think this is a very wordy way of saysing: "it seems to me we should return an error here" (unless I miss something of course which is entriely possible)

[lzap]

Good call, explicit error it is.

[mvo5]

(nipick) in general our convention for imports is (in most go project btw):

• stdlib
• external third party
• our own includes

we are not very strict about it but here I would suggest to move it between "strings" and github.com/osbuild/images/internal/common" (with \n before/after)

[mvo5]

I wonder if it wouldn't be better to also validate here, this would avoid the panic for invalid certs later. Here we could follow the pattern from e.g. GetRepositories() and return (*CACustomization,error) and then the callers (in fedora for example) can report a meaningful error for invalid cert strings early. Wdyt?

[lzap]

I can do that, just note this introduces a dependency on distro/manifest package. Also validating on a getter is something that can possibly cause performance issues later. Finally, getters are generally not recommended in Go communty.

Overall, I do not like this, but can do that we can discuss it over the code and I can revert the hunk if needed.

[lzap]

Ah it is an import cycle actually, I cannot do that, do you want me to do it? I would suggest to create an explicit CheckCACerts function and extract the parsing X509 code into a separate package.

[mvo5]

Thanks for looking into this, I'm not sure I follow, maybe I'm missing something but I was thinking of something like this:

diff --git a/pkg/blueprint/customizations.go b/pkg/blueprint/customizations.go
index 0bb5bacbe..f0d800077 100644
--- a/pkg/blueprint/customizations.go
+++ b/pkg/blueprint/customizations.go
@@ -428,12 +428,12 @@ func (c *Customizations) GetRHSM() *RHSMCustomization {
        return c.RHSM
 }
 
-func (c *Customizations) CheckCACerts() error {
+func (c *Customizations) checkCACerts() error {
        if c == nil {
                return nil
        }
 
-       if c.CACerts == nil {
+       if c.CACerts != nil {
                for _, bundle := range c.CACerts.PEMCerts {
                        _, err := cert.ParseCerts(bundle)
                        if err != nil {
@@ -445,9 +445,14 @@ func (c *Customizations) CheckCACerts() error {
        return nil
 }
 
-func (c *Customizations) GetCACerts() *CACustomization {
+func (c *Customizations) GetCACerts() (*CACustomization, error) {
        if c == nil {
-               return nil
+               return nil, nil
        }
-       return c.CACerts
+
+       if err := c.checkCACerts(); err != nil {
+               return nil, err
+       }
+
+       return c.CACerts, nil
 }
diff --git a/pkg/distro/fedora/images.go b/pkg/distro/fedora/images.go
index b0e737468..4c04a06c4 100644
--- a/pkg/distro/fedora/images.go
+++ b/pkg/distro/fedora/images.go
@@ -226,10 +226,11 @@ func osCustomizations(
        osc.Files = append(osc.Files, imageConfig.Files...)
        osc.Directories = append(osc.Directories, imageConfig.Directories...)
 
-       if err := c.CheckCACerts(); err != nil {
+       ca, err := c.GetCACerts()
+       if err != nil {
                panic(fmt.Sprintf("unexpected error checking CA certs: %v", err))
        }
-       if ca := c.GetCACerts(); ca != nil {
+       if ca != nil {
                osc.CACerts = ca.PEMCerts
        }
 
diff --git a/pkg/distro/rhel/images.go b/pkg/distro/rhel/images.go
index 5e13afe7f..806463b92 100644
--- a/pkg/distro/rhel/images.go
+++ b/pkg/distro/rhel/images.go
@@ -271,10 +271,11 @@ func osCustomizations(
                osc.NoBLS = *imageConfig.NoBLS
        }
 
-       if err := c.CheckCACerts(); err != nil {
+       ca, err := c.GetCACerts()
+       if err != nil {
                panic(fmt.Sprintf("unexpected error checking CA certs: %v", err))
        }
-       if ca := c.GetCACerts(); ca != nil {
+       if ca != nil {
                osc.CACerts = ca.PEMCerts
        }
 

i.e. every time we call GetCACerts() we implicitly check as well - this way we will not forget and it's similar to the pattern of GetRepositories() ?

[mvo5]

A simple unit test for this would be very nice, ideally both for a "positive" and "error" case, something like

// taken from osbuild:test/data/certs/cert1.pem
const exampleCert = `
-----BEGIN CERTIFICATE-----
MIIDhTCCAm2gAwIBAgIUVya7VJ3O8W8SqwuEa0BZ4HSsXvAwDQYJKoZIhvcNAQEL
BQAwUTELMAkGA1UEBhMCREUxDzANBgNVBAgMBkJlcmxpbjEPMA0GA1UEBwwGQmVy
bGluMQwwCgYDVQQKDANPcmcxEjAQBgNVBAMMCWxvY2FsaG9zdDAgFw0yNDA4MjYx
MDQyNDBaGA8yMTI0MDgwMjEwNDI0MFowUTELMAkGA1UEBhMCREUxDzANBgNVBAgM
BkJlcmxpbjEPMA0GA1UEBwwGQmVybGluMQwwCgYDVQQKDANPcmcxEjAQBgNVBAMM
CWxvY2FsaG9zdDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAJnGjlvN
O3F/Z7Lr/r+6Xp2DosnNwoPHhG2e61KnFzgZfaxbklal5ORpuV/gLIg7lrbpdZe7
WvK+16RanL6fLitis/tYVFyvz1MXqBYYrEoFGvVg9fOiis7hjpdZcpNDH9SngoAN
O0Wvv4T6LQS0cC7ZAFZjvmJ+RiZEbzRkNG5pUddZXbotE6htNfLgA5L1wIBgllrM
4DVkG0yNKmzqPNzfPTbdUgWCfjaQShHy1GP8KNEwFxM31F2wvQxsEb77o1S44Out
mlsi83tti6P7KjDk7w2j2zZO1X0xI8pflv3TBkJT1Am8vnk6rVnNO4pCpop3+kma
pDUEzBQmSQA5R1ECAwEAAaNTMFEwHQYDVR0OBBYEFDxFcFgPEsgsDixfKxB0uYGN
aJmzMB8GA1UdIwQYMBaAFDxFcFgPEsgsDixfKxB0uYGNaJmzMA8GA1UdEwEB/wQF
MAMBAf8wDQYJKoZIhvcNAQELBQADggEBAFih4lUbLlhKwIAV9x3/W7Mih8xUEdZr
olquZgaHedFet+ByAHvoES3pec7AVYTOD53mjgyZubD6INnVHzKyS4AG9ydD73o4
cmm3DKxBaesvlHeTn0MOKsoM8QCxeyFJmiUPpgDBok/PFnbGR9+JcsrlGJAnsSKD
vWpiwYcBauZ9nnK5yDe5M9XNFPkNDZzbKvWU7Sw3ziMT/+bRJse5vTrYcyOnNGgy
gZNz2nimKy1U8XZVAVwOV0rdGEFrfMln8DkRW86rGK/EncaVsl0SSP/rmjQgiX8Q
3CZraQGujJP932HSwUfdCX9yh+rTjE3MEnbqMoLzJa4BXB2aDQWtywU=
-----END CERTIFICATE-----
`

func TestParseCerts(t *testing.T) {
	certs := parseCerts(exampleCert)
	assert.Equal(t, len(certs), 1)
	assert.Equal(t, "localhost", certs[0].Subject.CommonName)
}

maybe?

[lzap]

I have extracted the checking code + test into separate package cert and implemented an explicit check method. No warnings but errors.

[mvo5]

Thanks for the update, looks very nice now, I made a suggestion inline, hope it's useful.

[mvo5]

This should be !=, no? If so ideally that would have triggered a test failure :/

[mvo5]

Thanks for looking into this, I'm not sure I follow, maybe I'm missing something but I was thinking of something like this:

diff --git a/pkg/blueprint/customizations.go b/pkg/blueprint/customizations.go
index 0bb5bacbe..f0d800077 100644
--- a/pkg/blueprint/customizations.go
+++ b/pkg/blueprint/customizations.go
@@ -428,12 +428,12 @@ func (c *Customizations) GetRHSM() *RHSMCustomization {
        return c.RHSM
 }
 
-func (c *Customizations) CheckCACerts() error {
+func (c *Customizations) checkCACerts() error {
        if c == nil {
                return nil
        }
 
-       if c.CACerts == nil {
+       if c.CACerts != nil {
                for _, bundle := range c.CACerts.PEMCerts {
                        _, err := cert.ParseCerts(bundle)
                        if err != nil {
@@ -445,9 +445,14 @@ func (c *Customizations) CheckCACerts() error {
        return nil
 }
 
-func (c *Customizations) GetCACerts() *CACustomization {
+func (c *Customizations) GetCACerts() (*CACustomization, error) {
        if c == nil {
-               return nil
+               return nil, nil
        }
-       return c.CACerts
+
+       if err := c.checkCACerts(); err != nil {
+               return nil, err
+       }
+
+       return c.CACerts, nil
 }
diff --git a/pkg/distro/fedora/images.go b/pkg/distro/fedora/images.go
index b0e737468..4c04a06c4 100644
--- a/pkg/distro/fedora/images.go
+++ b/pkg/distro/fedora/images.go
@@ -226,10 +226,11 @@ func osCustomizations(
        osc.Files = append(osc.Files, imageConfig.Files...)
        osc.Directories = append(osc.Directories, imageConfig.Directories...)
 
-       if err := c.CheckCACerts(); err != nil {
+       ca, err := c.GetCACerts()
+       if err != nil {
                panic(fmt.Sprintf("unexpected error checking CA certs: %v", err))
        }
-       if ca := c.GetCACerts(); ca != nil {
+       if ca != nil {
                osc.CACerts = ca.PEMCerts
        }
 
diff --git a/pkg/distro/rhel/images.go b/pkg/distro/rhel/images.go
index 5e13afe7f..806463b92 100644
--- a/pkg/distro/rhel/images.go
+++ b/pkg/distro/rhel/images.go
@@ -271,10 +271,11 @@ func osCustomizations(
                osc.NoBLS = *imageConfig.NoBLS
        }
 
-       if err := c.CheckCACerts(); err != nil {
+       ca, err := c.GetCACerts()
+       if err != nil {
                panic(fmt.Sprintf("unexpected error checking CA certs: %v", err))
        }
-       if ca := c.GetCACerts(); ca != nil {
+       if ca != nil {
                osc.CACerts = ca.PEMCerts
        }
 

i.e. every time we call GetCACerts() we implicitly check as well - this way we will not forget and it's similar to the pattern of GetRepositories() ?

[mvo5]

I would love to see a test for this but I understand this area is really difficult to test so fine to ignore that (but I also can't help to mention it)