enhancements/update/available-update-metadata: Propose a new enhancement #123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
the
size/L
wking
force-pushed
the
available-update-metadata
branch
from
594521f
to
fb0f876
Compare
|
/cc @smarterclayton |
|
|
||
| ### Risks and Mitigations | ||
|
|
||
| There is no security risk, because this is only exposing through ClusterVersion information that is already public via direct Cincinnati requests. |
There was a problem hiding this comment.
I don’t think this is correct. An attacker can perform a downgrade attack by suggesting an older version, and by suggesting the version is a critics secuirty update.
I might prefer to require all available updates to be extracted and validated before they can be candidate for cli selection (as another api object).
There was a problem hiding this comment.
An attacker can perform a downgrade attack by suggesting an older version, and by suggesting the version is a critics secuirty update.
Orthogonal? While storing Cincinnati metadata in availableUpdates would expose unsigned information (as discussed in this thread), that metadata would not be used for selecting choices out of availableUpdates. Selecting an update out of availableUpdates only uses the existing version and image properties, and this enhancement proposal isn't touching those.
There was a problem hiding this comment.
I guess a malicious/compromised Cincinnati could trick a cluster/user into visiting an erroneous errata page (if folks decided to click through to a maliciously injected url metadata), and content on that page could exploit a flaw in the admin's browser to do something malicious. Is it worth writing this angle up? I'd rank the risk well below "Mallory who is already running Cincinnati can suggest signed but unstable edge with an upgrade risk" or "Mallory can deny updates to the cluster and wait until some exploit turns up against the older version".
Or a malicious/compromised Cincinnati could trick a cluster/user into believing that a given release image belonged to an arbitrary channel. But if Mallory is already running Cincinnati, he can feed the cluster whatever graph he wants regardless of the channel sent up with the request, so I don't see an additional risk to exposing channel metadata.
There was a problem hiding this comment.
I don’t think this is correct. An attacker can perform a downgrade attack by suggesting an older version, and by suggesting the version is a critics secuirty update.
I might prefer to require all available updates to be extracted and validated before they can be candidate for cli selection (as another api object).
@smarterclayton can you elaborate your downgrade attack concern?
I don't yet understand how this new information can lead to such concern.
There was a problem hiding this comment.
I expect the downgrade concern is something like:
We have no CVO-side downgrade guards today. But the fact that today all updates are explicit decisions taken by cluster admins is something like a downgrade guard. Those admins are likely making their decisions based on the claimed version string, and not double checking by looking inside the by-digest pullspec referenced images. So today a malicious Cincinnati could say "I recommend you update to 4.4.5" and provide the 4.1.0 pullspec.
But I agree that that concern is orthogonal to this enhancement, since it's really about the source of the existing version property, which this enhancement does not touch. It's possible that the CVO could improve the reliability of the version property by only filling it with a version which was extracted from the release image, but I think the CVO can make that change unilaterally without going through an API-schema-change enhancement process.
wking
force-pushed
the
available-update-metadata
branch
from
fb0f876
to
63745e6
Compare
wking
added a commit
to wking/cincinnati-graph-data
that referenced
this pull request
Mar 12, 2020
So that Cincinnati doesn't complain when the console bumps its hard-coded list of channels and starts asking for stable-4.4 ([1] is in flight to remove the need for the console to hard-code available channels, but hasn't seen much recent progress). And even without the console issue, the installer has been putting new clusters in stable-4.4 since openshift/installer@d7fb12c07a (bootkube: Update to stable-4.4 channel, 2020-01-16, openshift/installer#2940). [1]: openshift/enhancements#123
This was referenced Mar 12, 2020
wking
force-pushed
the
available-update-metadata
branch
from
63745e6
to
bab98f6
Compare
|
I'm more interested in getting metadata on the current release (e.g. which Cincinnati channels does the current release belong to). Spinning out available releases into a separate object sounds fine, but also seems like a mostly orthogonal enhancement. Can I reroll this one to leave available updates untouched for now, and leave tweaking that up to follow-up work? Or are you ok with the tweaks I'm adding now landing as a step in the right direction? |
sdodson
pushed a commit
to sdodson/cincinnati-graph-data
that referenced
this pull request
Apr 16, 2020
So that Cincinnati doesn't complain when the console bumps its hard-coded list of channels and starts asking for stable-4.4 ([1] is in flight to remove the need for the console to hard-code available channels, but hasn't seen much recent progress). And even without the console issue, the installer has been putting new clusters in stable-4.4 since openshift/installer@d7fb12c07a (bootkube: Update to stable-4.4 channel, 2020-01-16, openshift/installer#2940). [1]: openshift/enhancements#123
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Apr 23, 2020
…ancement Rerolled to add Abinav's separate-object approach as an alternative [1]. [1]: openshift#123 (comment)
wking
force-pushed
the
available-update-metadata
branch
from
bab98f6
to
e870835
Compare
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Apr 27, 2020
…ancement Rerolled to add Abinav's separate-object approach as an alternative [1]. [1]: openshift#123 (comment)
wking
force-pushed
the
available-update-metadata
branch
from
e870835
to
182442f
Compare
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 20, 2020
With Abinav's separate-object approach as an alternative [1], and Clayton's pass-through rejection [2]. [1]: openshift#123 (comment) [2]: openshift#123 (comment)
wking
force-pushed
the
available-update-metadata
branch
from
6633aca
to
e92a521
Compare
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 20, 2020
With Abinav's separate-object approach as an alternative [1], and Clayton's pass-through rejection [2]. [1]: openshift#123 (comment) [2]: openshift#123 (comment)
wking
force-pushed
the
available-update-metadata
branch
from
e92a521
to
eba7c52
Compare
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 20, 2020
With Abinav's separate-object approach as an alternative [1], and Clayton's pass-through [2] and homepage [3] rejections. [1]: openshift#123 (comment) [2]: openshift#123 (comment) [3]: openshift#123 (comment)
wking
force-pushed
the
available-update-metadata
branch
from
eba7c52
to
63c6e7d
Compare
With Abinav's separate-object approach as an alternative [1], and Clayton's pass-through [2] and homepage [3] rejections. [1]: openshift#123 (comment) [2]: openshift#123 (comment) [3]: openshift#123 (comment)
wking
force-pushed
the
available-update-metadata
branch
from
63c6e7d
to
2eb16cf
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LalatenduMohanty, sdodson, wking 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Jul 21, 2020
This commit adds the ability to store release metadata, implementing openshift/enhancements@2eb16cf80d (enhancements/update/available-update-metadata: Propose a new enhancement, 2020-07-20, openshift/enhancements#123). For example the list of channels that a release is in: $ curl -sH 'Accept:application/json' 'https://api.openshift.com/api/upgrades_info/v1/graph?channel=stable-4.2' | jq -r '.nodes[] | select(.version == "4.2.4").metadata["io.openshift.upgrades.graph.release.channels"] | split(",")[]' candidate-4.2 fast-4.2 stable-4.2 That will allow the console and other downstream tooling to expose a list of channels available to a given cluster right now, instead of hard-coding an expected list [1]. This will account for things like phased releases, where we may not recommend an upgrade for your particular stable-4.2 cluster even though we are recommending the upgrade for other stable-4.2 clusters. Pivoting around this retyping in Go will require consumer bumps, but I don't think it's a breaking change because the only YAML removal is 'force', which is meaningless in AvailableUpdates. So I don't think this needs to count as a backwards-compat-breaking schema bump. Autogenerated bumps via: $ unset GOBIN # vendor/k8s.io/code-generator/generate-groups.sh and such require "${GOPATH}/bin/deepcopy-gen" $ go get k8s.io/gengo/examples/deepcopy-gen $ hack/update-deepcopy.sh $ hack/update-swagger-docs.sh $ make update-codegen-crds [1]: openshift/console#2935
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 21, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata: Propose a new enhancement, 2019-11-19, openshift#123): * Including two sentences from Clayton about how the CVO sources this information and how we build CI/nightly images [1]. * Downcasing in the Go comments, to match the JSON property, following the Kubernetes API pattern. * 'despire' -> 'despite' typo fix. [1]: openshift/api#521 (comment)
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 21, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata: Propose a new enhancement, 2019-11-19, openshift#123): * Including two sentences from Clayton about how the CVO sources this information and how we build CI/nightly images [1]. I think these two sentences are poking holes in the abstraction that the CVO is committing to with the new API, and that consumers should not have to know or care about these implementation details. Clayton feels like they provide useful context about how the system works, and that we'll adjust them as necessary if we ever have cause to alter the implementation. * Downcasing in the Go comments, to match the JSON property, following the Kubernetes API pattern. * 'despire' -> 'despite' typo fix. [1]: openshift/api#521 (comment)
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 21, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata: Propose a new enhancement, 2019-11-19, openshift#123): * Including a sentence from Clayton about how we currently `build CI/nightly images [1]. I think this sentence is poking holes in the abstraction that the CVO is committing to with the new API, and that consumers should not have to know or care about these implementation details. Clayton feels like it provides useful context about how the system works, and that we'll adjust them as necessary if we ever have cause to alter the implementation. I am rejecting his "This URL is set by the 'url' metadata property on a release..." suggestion [1], because the enhancement proposal includes: Then, in the cluster-version operator (CVO), this enhancement proposes porting existing logic like available-update translation and available-update lookup to preserve the Cincinnati metadata, with information from the release image itself taking precedence over information from Cincinnati. That information will populate url from the release image when the release image declares a url (always, for official OpenShift releases), but fall back to Cincinnati if it is not set in the release. Clayton's sentence does not talk about the Cincinnati fallback possibility. I am rejecting his "...should be displayed as a link in user interfaces" because interfaces that want to display a link will already know that this is their most-generic choice based on the other docstring sentences. But I'm fine including this fragment if folks feel like it is adding something useful on its own, with the "metadata property on a release" portion removed. * Downcasing in the Go comments, to match the JSON property, following the Kubernetes API pattern. * 'despire' -> 'despite' typo fix. [1]: openshift/api#521 (comment)
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 21, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata: Propose a new enhancement, 2019-11-19, openshift#123): * Including two sentences from Clayton about how the CVO sources this information and how we build CI/nightly images [1,2]. I think these two sentences are poking holes in the abstraction that the CVO is committing to with the new API, and that consumers should not have to know or care about these implementation details. Clayton feels like they provide useful context about how the system works, and that we'll adjust them as necessary if we ever have cause to alter the implementation. * Downcasing in the Go comments, to match the JSON property, following the Kubernetes API pattern. * 'despire' -> 'despite' typo fix. [1]: openshift/api#521 (comment) [2]: openshift/api#521 (comment)
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 24, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata: Propose a new enhancement, 2019-11-19, openshift#123): * Including two sentences from Clayton about how the CVO sources this information and how we build CI/nightly images [1,2]. I think these two sentences are poking holes in the abstraction that the CVO is committing to with the new API, and that consumers should not have to know or care about these implementation details. Clayton feels like they provide useful context about how the system works, and that we'll adjust them as necessary if we ever have cause to alter the implementation. * Downcasing in the Go comments, to match the JSON property, following the Kubernetes API pattern. * 'despire' -> 'despite' typo fix. [1]: openshift/api#521 (comment) [2]: openshift/api#521 (comment)
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 24, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata: Propose a new enhancement, 2019-11-19, openshift#123): * Including two sentences from Clayton about how the CVO sources this information and how we build CI/nightly images [1,2]. I think these two sentences are poking holes in the abstraction that the CVO is committing to with the new API, and that consumers should not have to know or care about these implementation details. Clayton feels like they provide useful context about how the system works, and that we'll adjust them as necessary if we ever have cause to alter the implementation. * Downcasing in the Go comments, to match the JSON property, following the Kubernetes API pattern. * 'despire' -> 'despite' typo fix. [1]: openshift/api#521 (comment) [2]: openshift/api#521 (comment)
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 24, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata: Propose a new enhancement, 2019-11-19, openshift#123): * Including two sentences from Clayton about how the CVO sources this information and how we build CI/nightly images [1,2]. I think these two sentences are poking holes in the abstraction that the CVO is committing to with the new API, and that consumers should not have to know or care about these implementation details. Clayton feels like they provide useful context about how the system works, and that we'll adjust them as necessary if we ever have cause to alter the implementation. * Downcasing in the Go comments, to match the JSON property, following the Kubernetes API pattern. * 'despire' -> 'despite' typo fix. [1]: openshift/api#521 (comment) [2]: openshift/api#521 (comment)
wking
added a commit
to wking/openshift-enhancements
that referenced
this pull request
Jul 24, 2020
Fixups to 2eb16cf (enhancements/update/available-update-metadata: Propose a new enhancement, 2019-11-19, openshift#123): * Including two sentences from Clayton about how the CVO sources this information and how we build CI/nightly images [1,2]. I think these two sentences are poking holes in the abstraction that the CVO is committing to with the new API, and that consumers should not have to know or care about these implementation details. Clayton feels like they provide useful context about how the system works, and that we'll adjust them as necessary if we ever have cause to alter the implementation. * Downcasing in the Go comments, to match the JSON property, following the Kubernetes API pattern. * 'despire' -> 'despite' typo fix. [1]: openshift/api#521 (comment) [2]: openshift/api#521 (comment)
wking
added a commit
to wking/openshift-api
that referenced
this pull request
Jul 24, 2020
This commit adds the ability to store release metadata, implementing openshift/enhancements@2eb16cf80d (enhancements/update/available-update-metadata: Propose a new enhancement, 2020-07-20, openshift/enhancements#123) as adjusted by openshift/enhancements@12eff48079 (enhancements/update/available-update-metadata: 'url' docstring context, 2020-07-21, openshift/enhancements#408). For example the list of channels that a release is in: $ curl -sH 'Accept:application/json' 'https://api.openshift.com/api/upgrades_info/v1/graph?channel=stable-4.2' | jq -r '.nodes[] | select(.version == "4.2.4").metadata["io.openshift.upgrades.graph.release.channels"] | split(",")[]' candidate-4.2 fast-4.2 stable-4.2 That will allow the console and other downstream tooling to expose a list of channels available to a given cluster right now, instead of hard-coding an expected list [1]. This will account for things like phased releases, where we may not recommend an upgrade for your particular stable-4.2 cluster even though we are recommending the upgrade for other stable-4.2 clusters. Pivoting around this retyping in Go will require consumer bumps, but I don't think it's a breaking change because the only YAML removal is 'force', which is meaningless in AvailableUpdates. So I don't think this needs to count as a backwards-compat-breaking schema bump. Autogenerated bumps via: $ unset GOBIN # vendor/k8s.io/code-generator/generate-groups.sh and such require "${GOPATH}/bin/deepcopy-gen" $ go get k8s.io/gengo/examples/deepcopy-gen $ hack/update-deepcopy.sh $ hack/update-swagger-docs.sh $ make update-codegen-crds [1]: openshift/console#2935
sdodson
pushed a commit
to sdodson/cincinnati-graph-data
that referenced
this pull request
Jul 29, 2020
So that Cincinnati doesn't complain when the console bumps its hard-coded list of channels and starts asking for stable-4.4 ([1] is in flight to remove the need for the console to hard-code available channels, but hasn't seen much recent progress). And even without the console issue, the installer has been putting new clusters in stable-4.4 since openshift/installer@d7fb12c07a (bootkube: Update to stable-4.4 channel, 2020-01-16, openshift/installer#2940). [1]: openshift/enhancements#123
sdodson
pushed a commit
to sdodson/cincinnati-graph-data
that referenced
this pull request
Aug 24, 2020
So that Cincinnati doesn't complain when the console bumps its hard-coded list of channels and starts asking for stable-4.4 ([1] is in flight to remove the need for the console to hard-code available channels, but hasn't seen much recent progress). And even without the console issue, the installer has been putting new clusters in stable-4.4 since openshift/installer@d7fb12c07a (bootkube: Update to stable-4.4 channel, 2020-01-16, openshift/installer#2940). [1]: openshift/enhancements#123
sdodson
pushed a commit
to sdodson/cincinnati-graph-data
that referenced
this pull request
Dec 3, 2020
So that Cincinnati doesn't complain when the console bumps its hard-coded list of channels and starts asking for stable-4.4 ([1] is in flight to remove the need for the console to hard-code available channels, but hasn't seen much recent progress). And even without the console issue, the installer has been putting new clusters in stable-4.4 since openshift/installer@d7fb12c07a (bootkube: Update to stable-4.4 channel, 2020-01-16, openshift/installer#2940). [1]: openshift/enhancements#123
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As @abhinavdahiya requested. CC @crawford, @spadgett, @steveej. Not sure who else would care about such a small pivot ;)