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.

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

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

lzap
Copy link
Contributor

@lzap lzap commented Oct 10, 2024

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.

The patch quite evolved during the review so editing the description so it is more appropriate.

cmd/ostree-resolve/main.go Outdated Show resolved Hide resolved
@lzap
Copy link
Contributor Author

lzap commented Oct 10, 2024

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

@lzap lzap requested a review from achilleas-k October 10, 2024 16:19
@lzap lzap force-pushed the mtls-ostree branch 2 times, most recently from d75dffc to 9505bf9 Compare October 15, 2024 07:05
@lzap lzap marked this pull request as ready for review October 15, 2024 07:05
Copy link
Member

@croissanne croissanne left a 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 Show resolved Hide resolved
@lzap
Copy link
Contributor Author

lzap commented Oct 15, 2024

Good point, amending a change. I am trying hard not to break the API by introducing an option struct, but since the ResolveRef does not seem to be used in composer, I am taking the liberty on changing it. If you feel this is not okay and other components might be using that, I will introduce a complatibility function and leave the original untouched.

Edit: Fixed tests.

@lzap lzap force-pushed the mtls-ostree branch 2 times, most recently from 652d38b to 56a8132 Compare October 15, 2024 10:00
cmd/ostree-resolve/main.go Outdated Show resolved Hide resolved
pkg/ostree/ostree.go Outdated Show resolved Hide resolved
pkg/ostree/ostree.go Outdated Show resolved Hide resolved
@lzap lzap force-pushed the mtls-ostree branch 2 times, most recently from 46d5ed0 to 3b8187d Compare October 15, 2024 10:06
@lzap lzap changed the title ostree: drop RHSM for MTLS over ENV ostree: configurable MTLS config for ostree resolve Oct 15, 2024
@lzap
Copy link
Contributor Author

lzap commented Oct 15, 2024

So I changed the code so the API contract is not broken. Tests are passing now, except I think unrelevant ones.

@lzap
Copy link
Contributor Author

lzap commented Oct 16, 2024

CentOS Stream 10 - BaseOS                       647  B/s | 3.9 kB     00:06    
Errors during downloading metadata for repository 'baseos':
  - Downloading successful, but checksum doesn't match. Calculated: 09f9f3040a68ccceb349f04463cad23b3af1dc36ccf61c1fada52b9955a441742d8434dcb66f265924aed2d2b9dee3edaf16e0534fe475c6a5faf45db021d9a1(sha512)  Expected: f3765f12125ff6f09532cd5e74a0ab0bf0b275aa28794c1171b11ed247ed46fe60a7869d2ddc4658e31ac155632b4def908fd473050e6f68c32322212b921f62(sha512) 
Error: Failed to download metadata for repo 'baseos': Cannot download repomd.xml: Cannot download repodata/repomd.xml: All mirrors were tried

@lzap lzap force-pushed the mtls-ostree branch 2 times, most recently from 379ffe3 to 4ec713c Compare October 16, 2024 07:27
pkg/ostree/ostree.go Outdated Show resolved Hide resolved
pkg/ostree/ostree.go Outdated Show resolved Hide resolved
pkg/ostree/ostree.go Outdated Show resolved Hide resolved
@lzap
Copy link
Contributor Author

lzap commented Oct 17, 2024

Oh when working on the changes you proposed, I realized I completely missed this input type which maps to the SourceSpec:

// 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.

@lzap lzap force-pushed the mtls-ostree branch 3 times, most recently from c1db885 to 2a9348b Compare October 21, 2024 14:12
@lzap
Copy link
Contributor Author

lzap commented Oct 21, 2024

Okay I am breaking the API here dropping the RHSM field, secret and creating a new Connection field that can be used to pass CA, client cert and proxy. How about this? @achilleas-k

For the record, I tested the resolver which is present in this repo, works fine with both proxy and MTLS:

#!/bin/bash

GO_VERSION=1.21.13

export OSBUILD_SOURCES_OSTREE_SSL_CA_CERT=./.vscode/test-ca.crt
export OSBUILD_SOURCES_OSTREE_SSL_CLIENT_CERT=./.vscode/client.home.lan-client.crt
export OSBUILD_SOURCES_OSTREE_SSL_CLIENT_KEY=./.vscode/client.home.lan-client.key
export OSBUILD_SOURCES_OSTREE_PROXY=http://xxx:[email protected]

go$GO_VERSION run ./cmd/otk/osbuild-resolve-ostree-commit < ./.vscode/ostree-input.json

# ostree-input.json:
{
    "tree": {
        "URL": "https://builder.home.lan/ccb2194f-9876-4e76-9e64-a338a32df230/",
        "Ref": "fedora/40/x86_64/iot"
    }
}

Result:

{
  "tree": {
    "const": {
      "ref": "fedora/40/x86_64/iot",
      "url": "https://builder.home.lan/ccb2194f-9876-4e76-9e64-a338a32df230/",
      "checksum": "5a5d9e2c1fa0a88d1318264f58753a8a34f19a8561825fd13d69f0822af6e86d"
    }
  }
}

@lzap
Copy link
Contributor Author

lzap commented Oct 24, 2024

So I have rewritten the patch to match the merged osbuild PR. I will drop few comments.

cmd/build/main.go Outdated Show resolved Hide resolved
pkg/distro/fedora/images.go Outdated Show resolved Hide resolved
@lzap lzap force-pushed the mtls-ostree branch 2 times, most recently from 94bf924 to d0e28be Compare October 25, 2024 09:59
@lzap
Copy link
Contributor Author

lzap commented Oct 25, 2024

So I have vastly simplified the patch after our discussion on Slack. I am no longer touching cmd/ or pkg/distro. The ostree.SourceSpec type was extened with *MTLS and Proxy. MTLS info cannot be set together with RHSM, callers must choose one or the other. This allows to provide arbitrary MTLS certs or RHSM certs. Secrets are set accordingly.

croissanne
croissanne previously approved these changes Oct 25, 2024
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

LGTM! very nice

achilleas-k
achilleas-k previously approved these changes Oct 25, 2024
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

Couple of nitpicks and comments, but nothing worth blocking the PR over. Feel free to implement the suggested changes or ignore them if you don't agree.

pkg/ostree/ostree_test.go Outdated Show resolved Hide resolved
pkg/ostree/ostree.go Show resolved Hide resolved
pkg/ostree/ostree.go Outdated Show resolved Hide resolved
pkg/ostree/ostree.go Show resolved Hide resolved
cmd/otk/osbuild-resolve-ostree-commit/main.go Outdated Show resolved Hide resolved
@lzap
Copy link
Contributor Author

lzap commented Oct 25, 2024

Amended all your comments. I can squash into a single commit since I am renaming the function for no reason, it is such a tiny refactoring for a separate commit. Or squash during merge.

@lzap
Copy link
Contributor Author

lzap commented Oct 29, 2024

The SBOM error is not related, integration tests are failing that is expected. What is the next step?

@achilleas-k achilleas-k added this pull request to the merge queue Oct 30, 2024
Merged via the queue into osbuild:main with commit 33a9496 Oct 30, 2024
16 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants