-
Notifications
You must be signed in to change notification settings - Fork 50
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.
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
ostree: configurable MTLS config for ostree resolve #975
base: main
Are you sure you want to change the base?
Conversation
For the record, we want to use the same cert which is used to access RPM content, maybe there already is an ENV variable for that? @croissanne |
d75dffc
to
9505bf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mtls secrets need to be passed in the same way as https://github.com/osbuild/osbuild-composer/blob/main/cmd/osbuild-worker/jobimpl-depsolve.go.
So they're set in the worker config, and then just pass them along to the ostree resolve job.
pkg/ostree/ostree.go
Outdated
tlsConf := &tls.Config{} | ||
|
||
// If CA is set, load the CA certificate and add it to the TLS configuration. Otherwise, use the system CA. | ||
if caFilename := os.Getenv("OSBUILD_SOURCES_OSTREE_SSL_CA_CERT"); caFilename != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using env variables here, I think you should be able to just pass a struct or something containing the mtls secrets. https://github.com/osbuild/osbuild-composer/blob/main/cmd/osbuild-worker/jobimpl-ostree-resolve.go is the entrypoint here
Good point, amending a change. I am trying hard not to break the API by introducing an option struct, but since the Edit: Fixed tests. |
652d38b
to
56a8132
Compare
cmd/ostree-resolve/main.go
Outdated
ostree.WithResolveCA(os.Getenv("OSBUILD_SOURCES_OSTREE_SSL_CA_CERT")), | ||
ostree.WithResolveMTLSClient(os.Getenv("OSBUILD_SOURCES_OSTREE_SSL_CLIENT_CERT"), os.Getenv("OSBUILD_SOURCES_OSTREE_SSL_CLIENT_KEY")), | ||
ostree.WithResolveProxy(proxy), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Resolve
function is kept compatible with the old codebase in composer, the options are completely optional therefore passing just spec will work and in fact not use any MTLS configuration.
pkg/ostree/ostree.go
Outdated
@@ -141,56 +134,46 @@ func verifyChecksum(commit string) bool { | |||
// ResolveRef resolves the URL path specified by the location and ref | |||
// (location+"refs/heads/"+ref) and returns the commit ID for the named ref. If | |||
// there is an error, it will be of type ResolveRefError. | |||
func ResolveRef(location, ref string, consumerCerts bool, subs *rhsm.Subscriptions, ca *string) (string, error) { | |||
func ResolveRef(location, ref, caCert, cert, key string, proxy *url.URL) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible API break but composer
does not appear to use this directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to break the API, will just require a tweak in composer when updating images.
46d5ed0
to
3b8187d
Compare
So I changed the code so the API contract is not broken. Tests are passing now, except I think unrelevant ones. |
|
379ffe3
to
4ec713c
Compare
// ResolveRef is deprecated, use ResolveRefMTLS. | ||
func ResolveRef(location, ref string, consumerCerts bool, subs *rhsm.Subscriptions, ca *string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just break the API. The mess of keeping deprecated functions around isn't worth the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about I do a followup PR once this is integrated into composer? Because if I remove this function tests immediately breaks, I feel like I would make my life harder by straight-up breaking it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which tests break? The integration ones with composer? Those are non-required for a reason. They're meant as a warning to the PR creator that they will have to change osbuild-composer when updating.
How about I do a followup PR once this is integrated into composer?
I'd rather have one clean PR than two messy ones. The way it is now it's hard to review (though not too hard). I suspect that a single PR that makes the appropriate change would be easier for me to wrap my head around, rather than try to imagine what it will look like eventually.
URL string | ||
Ref string | ||
// RHSM is deprecated, use Resolve function with MTLS configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the mTLS configuration be added to the SourceSpec? I imagine that would make things simpler, but I haven't written it out to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah well I thought that SourceSpec
is a type I should not touch. Now that I think about it, I can add fields directly in there that is more simple. Let me try.
pkg/ostree/ostree.go
Outdated
func Resolve(source SourceSpec, opt ...ResolveOptionFn) (CommitSpec, error) { | ||
options := ResolveOptions{} | ||
for _, o := range opt { | ||
o(&options) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very odd pattern for something that could just be an extension of the SourceSpec
struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah well I thought that SourceSpec
is a type I should not touch. Now that I think about it, I can add fields directly in there that is more simple.
Oh when working on the changes you proposed, I realized I completely missed this input type which maps to the // Input represents the user-provided inputs that will be used to resolve an
// ostree commit ID.
type Input struct {
// URL of the repo where the commit can be fetched.
URL string `json:"url"`
// Ref to resolve.
Ref string `json:"ref"`
// Whether to use RHSM secrets when resolving and fetching the commit.
RHSM bool `json:"rhsm,omitempty"`
} So it was not complete at all, I fixed it. How about this? The solution now properly reads these ENV variables, I hope they are available. |
To complete edge service Pulp migration, we need to be able to resolve and download ostree commits from Pulp running behind
console.redhat.com
proxy which only supports MTLS for authentication. I have found that some MTLS support is already present but it does not make much sense why RHSM is used. I cannot figure out a workflow why ostree repo would be published via RHSM cert. I am not aware of any RH ostree content hosted via RH CDN.This this patch is a proposal to completely remove the RHSM from ostree pipeline and replace it with simple ENV variables for CA cert, client cert and key. We only need support for accessing Insights. This feature can be possibly used by anyone wanting MTLS for ostree from which ability to set a CA cert is I think pretty helpful feature for anyone.
Now, this will likely break things, when I search for
org.osbuild.rhsm.consumer
I see couple of places so please search with me and help me to find what else needs to be refactored. Thanks.Reuses variables from osbuild/osbuild-composer#4412