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

feat: support "Pro" archives #167

Merged
merged 13 commits into from
Nov 15, 2024
Merged

Conversation

letFunny
Copy link
Collaborator

In chisel.yaml, archive definitions can now use the "pro" value to specify Ubuntu Pro archives. The archives.<archive>.pro value currently accepts the following values: "fips", "fips-updates", "apps" and "infra". Any other values are ignored.

By default, Chisel will look for credentials in the /etc/apt/auth.conf.d/ directory, unless the environment variable CHISEL_AUTH_DIR is set. In which case, it will look for configuration files in that directory. The configuration files may only have the ".conf" extensions or no extensions, the format is described in https://manpages.debian.org/testing/apt/apt_auth.conf.5.en.html.

  • Have you signed the CLA?

rebornplusplus and others added 2 commits October 18, 2024 17:42
In chisel.yaml, archive definitions can now use the "pro" value to
specify Ubuntu Pro archives. The `archives.<archive>.pro` value
currently accepts the following values: "fips", "fips-updates", "apps"
and "infra". Any other values are ignored.

By default, Chisel will look for credentials in the
`/etc/apt/auth.conf.d/` directory, unless the environment variable
`CHISEL_AUTH_DIR` is set. In which case, it will look for
configuration files in that directory. The configuration files may only
have the ".conf" extensions or no extensions, the format is described
in https://manpages.debian.org/testing/apt/apt_auth.conf.5.en.html.
@letFunny letFunny added the Priority Look at me first label Oct 18, 2024
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Looking good in general. There are several minors, but nothing fundamental about the logic. Thanks for looking into these.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
internal/archive/archive.go Outdated Show resolved Hide resolved
internal/archive/archive.go Outdated Show resolved Hide resolved
internal/archive/archive.go Outdated Show resolved Hide resolved
internal/setup/setup.go Outdated Show resolved Hide resolved
internal/setup/setup.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for this @letFunny .

I'd propose something different here, just to avoid leaking the real Pro token.

Atm, you're attaching multiple times - 1 for the real-archive-tests job, within the GH runner; and the 1+ within the spread tasks.
An oversight or injection of malicious tests could compromise this token.

Instead, since Chisel only needs the APT creds, I'd propose the following flow:

  1. have an initial job that sets up the Pro subscription
  2. save the APT creds as a GitHub output
  3. share that output with the real-archive-tests job
  4. share that same output with the spread job too

You may raise the point: "you might still leak the APT creds". True. I'm not sure if those bearer tokens ever expire, but in any case, with the Pro token you can also get them, plus more.

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for the good work in this feature!

},
}

func archiveURL(pro, arch string) (string, *credentials, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Function looks good now, thanks.

@niemeyer niemeyer merged commit 6418c54 into canonical:main Nov 15, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority Look at me first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants