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

OCPNODE-2439: Add support for BYOPKI #1658

Merged
merged 1 commit into from
Sep 19, 2024
Merged

OCPNODE-2439: Add support for BYOPKI #1658

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
110 changes: 91 additions & 19 deletions ...api-review/add-ClusterImagePolicy-and-ImagePolicy-for-Signature-Verification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ tools, so that I can utilize the increased security of my software supply chain.

### Goals

- ClusterImagePolicy is defined as cluster scoped CRD. ImagePolicy is defined as namespaced CRD.
- ClusterImagePolicy is defined as cluster scoped CRD. ImagePolicy is defined as namespaced CRD.
- The user can create an ImagePolicy instance specifying the images/repositories to be verified and their policy. The MCO will write the configuration for signature verification. Once this is done, CRI-O can verify the images/repositories.
- MCO container runtime config controller watches ImagePolicy instance in different kubernetes namespaces and merges the instances for each namespace into a single [containers-policy.json](https://github.com/containers/image/blob/main/docs/containers-policy.json.5.md) in a predefined `/path/to/policies/<NAMESPACE>.json`.
- MCO container runtime config controller watches ClusterImagePolicy instance and merges the instances into a single [/etc/containers/policy.json](https://github.com/containers/image/blob/main/docs/containers-policy.json.5.md).
Expand All @@ -45,7 +45,7 @@ tools, so that I can utilize the increased security of my software supply chain.
### Non-Goals

- Configuring policies for the images in the OCP payload is not within the scope of this enhancement.
- Providing a tool to mirror the signatures is out of the scope of this enhancement. In order to verify the signature, the disconnected users need to mirror signatures together with the application images.
- Providing a tool to mirror the signatures is out of the scope of this enhancement. In order to verify the signature, the disconnected users need to mirror signatures together with the application images.
- Grant the application administrator the ability to weaken cluster-scoped policies, to avoid expanding the set of administrators capable of increasing cluster exposure to vulnerable images.
- Grant the application administrator the ability to tighten cluster-scoped policies. This could be useful in the future, but we are deferring it to limit the amount of work needed for an initial implementation.

Expand All @@ -56,21 +56,21 @@ tools, so that I can utilize the increased security of my software supply chain.
**cluster administrator** Signature Verification Configuration Workflow:
1. The cluster administrator requests the addition of signature verification configurations at the cluster scope.
2. The cluster administrator writes the verification certification to the ClusterImagePolicy YAML file and creates a ClusterImagePolicy CR using `oc create -f imagepolicy.yaml`.
3. The cluster administrator can retrieve the merged cluster-scoped policies by checking `/etc/containers/policy.json` within the `99-<pool>-generated-registries` machine-config.
4. The cluster administrator has the option to delete the signature verification configuration by removing its ClusterImagePolicy instances.
3. The cluster administrator can retrieve the merged cluster-scoped policies by checking `/etc/containers/policy.json` within the `99-<pool>-generated-registries` machine-config.
4. The cluster administrator has the option to delete the signature verification configuration by removing its ClusterImagePolicy instances.

**application administrator** Signature Verification Configuration Workflow:
1. The application administrator requests the addition of signature verification configurations at the namespace scope.
2. The application administrator writes the verification certification to the ImagePolicy YAML file and creates a ImagePolicy CR using `oc create -f imagepolicy.yaml`.
Please note that the application administrator cannot override cluster-scoped policies, as they are treated with higher priority. The [Implementation Details](#Update-container-runtime-config-controller-to-watch-ClusterImagePolicy-and-ImagePolicy) explains the conflict resolution rules.
3. The application administrator can retrieve the cluster override and merged policies by checking `<NAMESPACE>.json` within the `99-<pool>-generated-registries` machine-config.
Please note that the application administrator cannot override cluster-scoped policies, as they are treated with higher priority. The [Implementation Details](#Update-container-runtime-config-controller-to-watch-ClusterImagePolicy-and-ImagePolicy) explains the conflict resolution rules.
3. The application administrator can retrieve the cluster override and merged policies by checking `<NAMESPACE>.json` within the `99-<pool>-generated-registries` machine-config.
4. The application administrator has the option to remove the signature verification configuration by deleting its ImagePolicy instances.

### API Extensions

#### Type definitions

Type definitions of ImagePolicy. ClusterImagePolicy is expected to have a similar structure to ImagePolicy.
Type definitions of ImagePolicy. ClusterImagePolicy is expected to have a similar structure to ImagePolicy.

```go
// +genclient
Expand Down Expand Up @@ -151,6 +151,10 @@ type PolicyRootOfTrust struct {
// https://github.com/sigstore/fulcio and https://github.com/sigstore/rekor
// +optional
FulcioCAWithRekor *FulcioCAWithRekor `json:"fulcioCAWithRekor,omitempty"`

// PKI defines the root of trust based on Root CA(s) and corresponding intermediate certificates.
// +optional
PKI *PKI `json:"pki,omitempty"`
Comment on lines +154 to +157
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's fix the indentation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated. thanks.

}

// +kubebuilder:validation:Enum=PublicKey;FulcioCAWithRekor
Expand All @@ -159,6 +163,7 @@ type PolicyType string
const (
PublicKeyRootOfTrust PolicyType = "PublicKey"
FulcioCAWithRekorRootOfTrust PolicyType = "FulcioCAWithRekor"
PKIRootOfTrust PolicyType = "PKI"
)

// PublicKey defines the root of trust based on a sigstore public key.
Expand Down Expand Up @@ -192,6 +197,36 @@ type FulcioCAWithRekor struct {
FulcioSubject PolicyFulcioSubject `json:"fulcioSubject,omitempty"`
}

// PKI defines the root of trust based on Root CA(s) and corresponding intermediate certificates.
type PKI struct {
harche marked this conversation as resolved.
Show resolved Hide resolved
// CertificateAuthorityRootsData contains base64-encoded data of a certificate bundle PEM file, which contains one or more CA roots in the PEM format.
// +required
CertificateAuthorityRootsData []byte `json:"caRootsData,omitempty"`

// CertificateAuthorityIntermediatesData base64-encoded data of a certificate bundle PEM file, which contains one or more intermediate certificates in the PEM format.
// caIntermediatesData requires CertificateAuthorityRoots to be set.
// +optional
// +kubebuilder:validation:XValidation:rule="self == '' || self.CertificateAuthorityRootsData != ''", message="caIntermediatesData requires caRootsData to be set"
harche marked this conversation as resolved.
Show resolved Hide resolved
CertificateAuthorityIntermediatesData []byte `json:"caIntermediatesData,omitempty"`

// PKICertificateSubject defines the requirements imposed on the subject to which the certificate was issued.
// +required
// +kubebuilder:validation:XValidation:rule="self.CertificateAuthorityRootsData == '' || self.PKICertificateSubject != nil", message="pkiCertificateSubject must be set if caRootsData is specified"
PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

how about just using CertificateSubject since we're already in the PKI struct?

Suggested change
PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject,omitempty"`
CertificateSubject *CertificateSubject `json:"certificateSubject,omitempty"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtrmac WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as correctness goes, sure, why not.

One concern to think about is that there are two audiences here:

  • Users writing YAML: for them the json field names matter, and we can rely on the nesting structure to avoid repetition. So certificateSubject is better than pkiCertificateSubject
  • Go programmers using the API package: all types are at the top level, so they should be unique standalone. So PKICertificateSubject is better than CertificateSubject, or maybe it should be PKIImagePolicyCertificateSubject in the extreme??
    • Note especially that the naming the top-level structure just PKI is not ideal… we can get away with that for FulcioCAWithRekor, but for PKI and the existing PublicKey… there’s some risk of clashes.

But, also, the YAML names are more important because they are user-facing; the Go type names are ~internal in a package with no stable API commitment (I think?) so if we ever encountered a conflict, we can rename things.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PKICertificateSubject *PKICertificateSubject `json:"pkiCertificateSubject,omitempty"`
PKICertificateSubject *PKICertificateSubject `json:"certificateSubject,omitempty"`

Maybe only omit the PKI from the json annotation.

Copy link
Member

Choose a reason for hiding this comment

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

Note especially that the naming the top-level structure just PKI is not ideal… we can get away with that for FulcioCAWithRekor, but for PKI and the existing PublicKey… there’s some risk of clashes.

will CustomPKI instead of PKI make a difference? we can explain in the comment this means a PKI not from Sigstore.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the context of openshift/api/config/v1alpha1 (or, worse, eventually, …/v1, I think PKI and CustomPKI are very similarly likely to clash — if there is going to be a clash, it’s going to be with some other non-policy.json PKI use.

See https://pkg.go.dev/github.com/openshift/api/config/v1 — there are things like NamedCertificate and CertInfo.

So I’d be tempted to use ImagePolicy* for all types (ImagePolicyPKI?), but that might well be an overreaction.

I do think the JSON/YAML field names are much more important; I’m not nearly as worried about the Go types.

}
harche marked this conversation as resolved.
Show resolved Hide resolved

// PKICertificateSubject defines the requirements imposed on the subject to which the certificate was issued.
type PKICertificateSubject struct {
// Email address associated with the certificate identity.
// +optional
// +kubebuilder:validation:XValidation:rule="self.Email != '' || self.Hostname != ''", message="At least one of Email or Hostname must be set in pkiCertificateSubject"
Email string `json:"email,omitempty"`

// Hostname associated with the certificate identity.
// +optional
Hostname string `json:"hostname,omitempty"`
}

// PolicyFulcioSubject defines the OIDC issuer and the email of the Fulcio authentication configuration.
type PolicyFulcioSubject struct {
// oidcIssuer contains the expected OIDC issuer. It will be verified that the Fulcio-issued certificate contains a (Fulcio-defined) certificate extension pointing at this OIDC issuer URL. When Fulcio issues certificates, it includes a value based on an URL inside the client-provided ID token.
Expand Down Expand Up @@ -284,7 +319,7 @@ type ImagePolicyStatus struct {

Enhance the MCO container runtime config controller to manage ClusterImagePolicy and ImagePolicy CRs, and update the signature verification configurations:
- Retrieves all the ClusterImagePolicy and ImagePolicy instances on the cluster.
- Pre-validation:
- Pre-validation:
- Ensure that there are no scopes under the OCP image payload. If any are found, add an info-level log message to the machine config controller about the error encountered while adding an OCP image payload. Update the `status` of the CR to indicate that the policy will not be applied.
- Check for conflicts between cluster scope and namespace scope policies. If the namespaced ImagePolicy scope is equal to or nests inside an existing cluster-scoped ClusterImagePolicy CR, do not deploy the namespaced policy.
Update the `status` of both CRs and the machine config controller logs to indicate that the ClusterImagePolicy will be applied, while the non-global ImagePolicy will not be applied.
Expand All @@ -303,13 +338,13 @@ Enhance the MCO container runtime config controller to manage ClusterImagePolicy
default-docker:
use-sigstore-attachments: true
```
- Merge rule for ClusterImagePolicy and ImagePolicy CRs:
- Merge rule for ClusterImagePolicy and ImagePolicy CRs:
- when the syncHandler processing an image scope within a ClusterImagePolicy CR
- the scope exists in an existing ClusterImagePolicy CR:
append the policy to existing cluster policy; the policy will be written to `/etc/containers/policy.json` and each `\<NAMESPACE\>.json` if that namespace has an ImagePolicy CR that can be successfully rolled out.
- if the scope is either equal to or a broader scope than one already present in an ImagePolicy CR: do not roll out the non-global policy in `<NAMESPACE>.json`. The policy for the cluster will be written to `/etc/containers/policy.json` and `<NAMESPACE>.json`;
update the `status` of both CRs and machine config controller logs to indicate that the policy for cluster has been applied, while the non-global namespace policy is ignored.
- if none of the above cases apply: the policy will be written to `/etc/containers/policy.json` and each `\<NAMESPACE\>.json` if that namespace has an ImagePolicy CR that can be successfully rolled out.
- if none of the above cases apply: the policy will be written to `/etc/containers/policy.json` and each `\<NAMESPACE\>.json` if that namespace has an ImagePolicy CR that can be successfully rolled out.
- when the syncHandler processing an image scope within an ImagePolicy CR
- if the policy scope is equal to or nests inside an existing ClusterImagePolicy CR: do not roll out the non-global policy in `\<NAMESPACE\>.json`. the policy for the cluster will be written to `/etc/containers/policy.json` and `\<NAMESPACE\>.json`;
update the `status` of both CRs and machine config controller logs to indicate that the policy for the cluster has been applied, while the non-global namespace policy is ignored.
Expand All @@ -334,7 +369,7 @@ Enhance the MCO container runtime config controller to manage ClusterImagePolicy
Example of ClusterImagePolicy and ImagePolicy.

```yaml
kind: ClusterImagePolicy
kind: ClusterImagePolicy
metadata:
name: mypolicy-0
spec:
Expand All @@ -357,7 +392,7 @@ spec:
```

```yaml
kind: ClusterImagePolicy
kind: ClusterImagePolicy
metadata:
name: mypolicy-1
spec:
Expand All @@ -384,14 +419,51 @@ metadata:
namespace: testnamespace
spec:
scopes:
- test0.com # for test0.com, cluster policy will overwrite this policy
- test0.com # for test0.com, cluster policy will overwrite this policy
- test2.com
policy:
rootoftrust:
policyType: PublicKey
publicKey:
keydata: dGVzdC1rZXktZGF0YQ==
```
```yaml
kind: ImagePolicy
metadata:
name: byopkipolicy # BYOPKI with Root CA
namespace: testnamespace
spec:
scopes:
- test0.com # for test0.com, cluster policy will overwrite this policy
- test2.com
policy:
rootoftrust:
policyType: PKI
pki:
caRootsData: dGVzdC1rZXktZGF0YQ==
pkiCertificateSubject:
email: test-user@test-domain.com
```
```yaml
kind: ImagePolicy
metadata:
name: byopkipolicy # BYOPKI with Root CA and Intermediate CA
namespace: testnamespace
spec:
scopes:
- test0.com # for test0.com, cluster policy will overwrite this policy
- test2.com
policy:
rootoftrust:
policyType: PKI
pki:
caRootsData: dGVzdC1rZXktZGF0YQ==
caIntermediatesData: dGVzdC1rZXktZGF0YL==
pkiCertificateSubject:
email: test-user@test-domain.com
```



Feedback from the container runtime config controller:

Expand Down Expand Up @@ -462,7 +534,7 @@ Apply the above CRs, if no Image CRs changes the policy.json. The below `/etc/co
}
```

The merged cluster override policy and namespaced policy in the below `/path/to/policies/testnamespace.json` will be rolled out.
The merged cluster override policy and namespaced policy in the below `/path/to/policies/testnamespace.json` will be rolled out.

```json
{
Expand Down Expand Up @@ -550,8 +622,8 @@ Mitigation: The [Implementation Details](#Update-container-runtime-config-contro

MCO container runtime config controller can add unit tests and e2e tests.
- unit test: verify the policies from ClusterImagePolicy and ImagePolicy instances merged correctly and the controller writes correct format policy.json
- e2e test:
- verify the MCO writes configuration to the correct location.
- e2e test:
- verify the MCO writes configuration to the correct location.
- test demonstrating that an unsigned image in violation of the policy is rejected


Expand Down Expand Up @@ -593,8 +665,8 @@ Upgrade expectations:
- In order to use ClusterImagePolicy and ImagePolicy, an existing cluster will have to make an upgrade to the version that the `ClusterImagePolicy` and `ImagePolicy` CRDs are available.
- If the existing cluster manages `/etc/containers/policy.json` via the MachineConfig, the configuration will continue to work if no new `ClusterImagePolicy` or `ImagePolicy` resources created during the upgrade. If `ImagePolicy` resources created and resulting `/path/to/policies/<NAMESPACE>.json` exists, CRI-O will use the config from `/path/to/policies/<NAMESPACE>.json`.

Downgrade expectations:
- Not applicable.
Downgrade expectations:
- Not applicable.
If `N` does not support ClusterImagePolicy and ImagePolicy CRD, `N+1`supports ClusterImagePolicy and ImagePolicy. It is not expected that the CRDs related to the failure `N->N+1`. New resources should not be created during `N->N+1`.


Expand All @@ -611,7 +683,7 @@ The implementation of `ClusterImagePolicy` and `ImagePolicy` is synchronized wit
- MCO rolling out configuration file failure reported by MCO
- If the signature validation fails, then CRI-O will report that in the same way as for any other pull failure. Further enhancements to the kubelet, CRI and CRI-O error handling can be achieved in future Kubernetes releases.

The above errors should not impact the overall cluster health since configuring policies for the images in the OCP payload is prohibited.
The above errors should not impact the overall cluster health since configuring policies for the images in the OCP payload is prohibited.
The OCP Node team is likely to be called upon in case of escalation with one of the failure modes.

#### Support Procedures
Expand Down