NE-310 Implement route admission support for HSTS header #958

candita · 2021-06-29T16:01:57Z

Route admission support for HSTS annotations on routes.

Add type RequiredHSTSPolicy with HSTS directives plus namespace selector and domain patterns to match. Add supporting variables.
Add RequiredHSTSPolicies to IngressSpec

Enhancement proposal: openshift/enhancements#749

config/v1/types.go

deads2k · 2021-06-30T18:59:34Z

config/v1/types.go

+	// tool for administrators to quickly correct mistakes.  The minimum `max-age` default is -1 instead of 0.
+	// If it is set to 1 in MaxAgePolicy, then the HSTS policy may not be deleted.
+	// omitempty
+	SmallestMaxAge int32 `json:"smallestMaxAge,omitempty"`


the doc on this field is very good. We normally try to avoid pointers because a distinction between zero and nil is painful for API consumers, but your doc here is very convincing. Do you think a *int32 provides a better experience and allows the default object (with nil) to mean no opinion?

Agreed. Made changes.

config/v1/types.go

config/v1/types_ingress.go

deads2k · 2021-06-30T19:04:34Z

config/v1/types_ingress.go

@@ -54,6 +54,22 @@ type IngressSpec struct {
 	// configurable routes.
 	// +optional
 	ComponentRoutes []ComponentRouteSpec `json:"componentRoutes,omitempty"`
+
+	// requiredHSTSPolicies specifies HSTS policies that are required to be set on


question about the logic here. I would have expected of the slice to matter, so evaluation would look like

check the route against each requiredHSTSPolicy index, in order

if the namespace selector and the domain pattern matches, the first item in requiredHSTSPolicy is the one used

the accept/reject is based on that single item

Such an ordering concept will allow for punch-throughs by placing different policy early in the slice.

Do you mean you'd like to see requiredHSTSPolicy indexed by namespace selector + domain pattern?

Ok, yes, it does use that logic. It is based on your WIP for the admission plugin. I have a slightly revised WIP for the admission plugin here: https://github.com/openshift/openshift-apiserver/pull/224/files#diff-cde26a88c8aa9772d3d1cbf96497ac0009504bc258fd662e2ad19b458dcd3d81
I'll change the comments here to make it clearer.

Right, the plugin logic looks like what David describes: Pick the first matching item (matching on domain and namespace label) and admit or reject based on that item. That also seems like a good way to describe in this godoc how the list of policies is applied. As it is, the description currently in the godoc is difficult to reason about because it has many cases and describes the matching logic in terms of matching the policy as well as the domain and the namespace label.

I updated the comments, hope it is clarified.

config/v1/types.go

deads2k · 2021-07-02T20:20:22Z

config/v1/types.go

+	// domainPatterns is an optional list of domains for which the desired HSTS annotations are required.
+	// If domainPatterns is specified and a route is created with a spec.host matching one of the domains,
+	// the route must specify the HSTS Policy components described in the matching RequiredHSTSPolicy.
+	// If domainPatterns is empty, the specified HSTS Policy components are IGNORED.


If the policy is ignored if domain patterns are empty, why would a cluster-admin want to create it? Should we simply make the minlength=1?

I could understand someone entering the policy, and later deleting the domainPattern as a cleanup step, but also wanting to keep it around in case a new domain wanted to use or test a proven policy.

It seems weird to let users use the API as a note-keeping tool. I agree with David that it makes more sense to set minlength=1. Setting minlength=1 would also make this comment on "namespaceSelector" superfluous: "A domain MUST be specified in order for namespaceSelector to apply".

Okay. I will also make DomainPatterns required.

config/v1/types_ingress.go

config/v1/types.go

deads2k · 2021-07-02T20:27:23Z

config/v1/types.go

+type MaxAgePolicy struct {
+	// The largest value of the RequiredHSTSPolicy max-age
+	// -1 means no opinion
+	// kubebuilder:validation:default=-1


add validation for a range please.

There is validation in the api-server PR openshift/openshift-apiserver#224

Does it also need to be here?

I added range validation.

It seems as if I cannot add range validation or a default if I make this *int32

deads2k · 2021-07-02T20:29:21Z

config/v1/types.go

+	// If it is set to 1 in MaxAgePolicy, then the HSTS policy header may not be inactivated unless it expires.
+	// -1 means no opinion.
+	// kubebuilder:validation:default=-1
+	SmallestMaxAge int32 `json:"smallestMaxAge,omitempty" protobuf:"varint,1,opt,name=largestMaxAge"`


The doc on this field reads like a very compelling reason to have a *int32 where nil and 0 mean different things. How do you feel about allowing nil and instead of -1, "I want the default" is expressed as nil?

Similar comment here: #958 (comment)

I saw it before too. I'm still thinking about this.

I agree -- will use nil instead of -1 on this. Does it sound right to make the default smallest max = 1 day and largest max = 1 year?

I decided it doesn't make sense to default these values. If they are missing, it indicates that the admin doesn't care.

Miciah · 2021-07-13T23:42:04Z

config/v1/types.go

+	// domainPatterns is an optional list of domains for which the desired HSTS annotations are required.
+	// If domainPatterns is specified and a route is created with a spec.host matching one of the domains,
+	// the route must specify the HSTS Policy components described in the matching RequiredHSTSPolicy.
+	// If domainPatterns is empty, the specified HSTS Policy components are IGNORED.


It seems weird to let users use the API as a note-keeping tool. I agree with David that it makes more sense to set minlength=1. Setting minlength=1 would also make this comment on "namespaceSelector" superfluous: "A domain MUST be specified in order for namespaceSelector to apply".

config/v1/types.go

Miciah · 2021-07-13T23:42:31Z

config/v1/types_ingress.go

+	// - If the candidate route doesn't match any requiredHSTSPolicy domainPattern and optional namespaceSelector,
+	// then it may use any HSTS Policy annotation.
+	//
+	// The HSTSRequiredPolicies may be updated over time and can invalidate a route that was previously admitted.


We need to be clear here that an existing route won't suddenly stop working, but updates to it may be rejected.

@Miciah what do you think of:

The HSTSRequiredPolicies may be changed over time and can therefore reject a previously admitted route if it has any invalidating changes after the HSTSRequiredPolicies are changed.

I'm still worried that that could be misinterpreted. How about, "The HSTS policy configuration may be changed after routes have already been created. An update to a previously admitted route may then fail if the updated route does not conform to the updated HSTS policy configuration. However, changing the HSTS policy configuration will not cause a route that is already admitted to stop working."

Miciah · 2021-07-14T21:28:55Z

config/v1/types.go

+// MaxAgePolicy contains a numeric range for specifying a compliant HSTS max-age for the enclosing RequiredHSTSPolicy
+type MaxAgePolicy struct {
+	// The largest allowed value (in seconds) of the RequiredHSTSPolicy max-age
+	// This is a pointer to distinguish between value of zero and value unspecified.


There are no pointers in json or yaml. Better to use more general terminology:

Suggested change

// This is a pointer to distinguish between value of zero and value unspecified.

// This value can be left unspecified, in which case no upper limit is enforced.

Miciah · 2021-07-16T15:26:27Z

config/v1/types.go

+
+// IncludeSubDomainsPolicy contains a value for specifying a compliant HSTS includeSubdomains policy
+// for the enclosing RequiredHSTSPolicy
+// +kubebuilder:validation:Enum=RequireIncludeSubDomains;RequireNotIncludeSubDomains;NoOpinion


Suggested change

// +kubebuilder:validation:Enum=RequireIncludeSubDomains;RequireNotIncludeSubDomains;NoOpinion

// +kubebuilder:validation:Enum=RequireIncludeSubDomains;RequireNoIncludeSubDomains;NoOpinion

Miciah · 2021-07-16T15:28:10Z

config/v1/types.go

+}
+
+// PreloadPolicy contains a value for specifying a compliant HSTS preload policy for the enclosing RequiredHSTSPolicy
+// +kubebuilder:validation:Enum=RequirePreload;RequireNotPreload;NoOpinion


Suggested change

// +kubebuilder:validation:Enum=RequirePreload;RequireNotPreload;NoOpinion

// +kubebuilder:validation:Enum=RequirePreload;RequireNoPreload;NoOpinion

Miciah · 2021-07-16T15:58:30Z

/lgtm

candita · 2021-07-16T19:03:55Z

/assign @sttts

deads2k · 2021-07-16T19:45:55Z

config/v1/types_ingress.go

+	// then it may use any HSTS Policy annotation.
+	//
+	// The HSTS policy configuration may be changed after routes have already been created. An update to a previously
+	// admitted route may then fail if the updated route does not conform to the updated HSTS policy configuration.


The admission plugin should only fail on a route update if hsts related values are changing. This statement is accurate if constrained to that case.

If you bump into some questions about how to make that ratchet, we can get into it on the admission PR.

deads2k · 2021-07-16T19:46:17Z

Thanks for the updates. Looks like a solid API.

/approve

openshift-ci · 2021-07-16T19:46:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita, deads2k, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from derekwaynecarr and jwforres June 29, 2021 16:02

sgreene570 reviewed Jun 29, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

sgreene570 reviewed Jun 29, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

sgreene570 reviewed Jun 29, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

sgreene570 reviewed Jun 29, 2021

View reviewed changes

config/v1/types.go Show resolved Hide resolved

sgreene570 reviewed Jun 29, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

sgreene570 reviewed Jun 29, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

deads2k reviewed Jun 30, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

deads2k reviewed Jun 30, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

deads2k reviewed Jun 30, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

deads2k reviewed Jun 30, 2021

View reviewed changes

config/v1/types_ingress.go Outdated Show resolved Hide resolved

deads2k reviewed Jun 30, 2021

View reviewed changes

config/v1/types_ingress.go Outdated Show resolved Hide resolved

deads2k reviewed Jun 30, 2021

View reviewed changes

candita force-pushed the NE-310-HSTS branch 2 times, most recently from 7e362f2 to 34abd20 Compare July 1, 2021 14:38

deads2k reviewed Jul 2, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

deads2k reviewed Jul 2, 2021

View reviewed changes

config/v1/types_ingress.go Outdated Show resolved Hide resolved

deads2k reviewed Jul 2, 2021

View reviewed changes

config/v1/types_ingress.go Outdated Show resolved Hide resolved

deads2k reviewed Jul 2, 2021

View reviewed changes

config/v1/types.go Outdated Show resolved Hide resolved

deads2k reviewed Jul 2, 2021

View reviewed changes

candita force-pushed the NE-310-HSTS branch 2 times, most recently from 8e06be2 to b61c93f Compare July 2, 2021 23:21

candita force-pushed the NE-310-HSTS branch from b61c93f to d3043f3 Compare July 13, 2021 15:53

Miciah reviewed Jul 13, 2021

View reviewed changes

candita force-pushed the NE-310-HSTS branch 2 times, most recently from 652844f to e3fbcd4 Compare July 14, 2021 18:18

candita force-pushed the NE-310-HSTS branch from e3fbcd4 to 3863fec Compare July 14, 2021 20:55

Miciah reviewed Jul 14, 2021

View reviewed changes

candita force-pushed the NE-310-HSTS branch 2 times, most recently from 471f0d3 to 4cefcc1 Compare July 15, 2021 00:16

Miciah reviewed Jul 16, 2021

View reviewed changes

NE-310 Implement route admission support for HSTS header

6669bbd

candita force-pushed the NE-310-HSTS branch from 4cefcc1 to 6669bbd Compare July 16, 2021 15:33

Miciah mentioned this pull request Jul 16, 2021

NE-310 Enhancement proposal for HSTS route admission plugin openshift/enhancements#749

Merged

openshift-ci bot assigned Miciah Jul 16, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2021

Miciah mentioned this pull request Jul 16, 2021

WIP: config/ingress: Add required route annotations API #881

Closed

openshift-ci bot assigned sttts Jul 16, 2021

deads2k reviewed Jul 16, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2021

openshift-merge-robot merged commit ed5c917 into openshift:master Jul 16, 2021

candita mentioned this pull request Jul 26, 2021

Bug 1986148: Bump API for Ingress RequiredHSTSPolicies openshift/cluster-config-operator#212

Merged

Miciah mentioned this pull request Aug 13, 2021

Bug 1993002: fix kubebuilder directives #988

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NE-310 Implement route admission support for HSTS header #958

NE-310 Implement route admission support for HSTS header #958

candita commented Jun 29, 2021

deads2k Jun 30, 2021

candita Jul 13, 2021

deads2k Jun 30, 2021

candita Jul 1, 2021 •

edited

Loading

candita Jul 1, 2021

Miciah Jul 2, 2021

candita Jul 2, 2021

deads2k Jul 2, 2021

candita Jul 2, 2021

Miciah Jul 13, 2021

candita Jul 14, 2021

deads2k Jul 2, 2021

candita Jul 2, 2021

candita Jul 13, 2021

candita Jul 14, 2021

deads2k Jul 2, 2021 •

edited

Loading

candita Jul 2, 2021

candita Jul 13, 2021

candita Jul 14, 2021

Miciah Jul 13, 2021

Miciah Jul 13, 2021

candita Jul 14, 2021

Miciah Jul 14, 2021

candita Jul 15, 2021

Miciah Jul 14, 2021

candita Jul 15, 2021

Miciah Jul 16, 2021

Miciah Jul 16, 2021

Miciah commented Jul 16, 2021

candita commented Jul 16, 2021

deads2k Jul 16, 2021

candita Jul 16, 2021

deads2k commented Jul 16, 2021

openshift-ci bot commented Jul 16, 2021

	// This is a pointer to distinguish between value of zero and value unspecified.
	// This value can be left unspecified, in which case no upper limit is enforced.

	// +kubebuilder:validation:Enum=RequireIncludeSubDomains;RequireNotIncludeSubDomains;NoOpinion
	// +kubebuilder:validation:Enum=RequireIncludeSubDomains;RequireNoIncludeSubDomains;NoOpinion

	// +kubebuilder:validation:Enum=RequirePreload;RequireNotPreload;NoOpinion
	// +kubebuilder:validation:Enum=RequirePreload;RequireNoPreload;NoOpinion

NE-310 Implement route admission support for HSTS header #958

NE-310 Implement route admission support for HSTS header #958

Conversation

candita commented Jun 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah commented Jul 16, 2021

candita commented Jul 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 16, 2021

openshift-ci bot commented Jul 16, 2021

candita Jul 1, 2021 •

edited

Loading

deads2k Jul 2, 2021 •

edited

Loading