[feature] support key by url in apt configure module

[ubuntu-server-builder]

This bug was originally filed in Launchpad as LP: #2006775

Launchpad details

affected_projects = []
assignee = None
assignee_name = None
date_closed = None
date_created = 2023-02-09T19:08:55.982578+00:00
date_fix_committed = None
date_fix_released = None
id = 2006775
importance = wishlist
is_complete = False
lp_url = https://bugs.launchpad.net/cloud-init/+bug/2006775
milestone = None
owner = holmanb
owner_name = Brett Holman
private = False
status = triaged
submitter = holmanb
submitter_name = Brett Holman
tags = ['bitesize']
duplicates = []

Launchpad user Brett Holman(holmanb) wrote on 2023-02-09T19:08:55.982578+00:00

A common way of distributing keys is by hosting them at a URL for download. This is not currently supported by the apt configure module, and would be a simple addition.

Example usage (note the suggested 'keyurl' key)

apt:
  sources:
      source1:
          keyurl: 'https://domain.io/keys/key1.gpg'
          source: 'deb [signed-by=$KEY_FILE] http://<url>/ jammy main'

[kaiwalyakoparkar]

I would like to work on this issue :)

[TheRealFalcon]

@kaiwalyakoparkar , we'll welcome the contribution. At the moment, nobody else is working on this, so feel free to work on it.

[kaiwalyakoparkar]

@TheRealFalcon Thank you so much! I would like to know about the issue and maybe what folder and files should I be looking at while working on this issue? Also references or other material to understand the issue would be appreciated :)

[aciba90]

The target is to add to the apt_configure module the functionality to fetch gpg keys from a URL and associate them to a pkg source list.

To do so, we would need to extend the config section associated to the module in the lines of the issue's example and implement it.

One way to implement it would be to use apt-key, but this command is deprecated, see the man page.

Please, let us know if you, @kaiwalyakoparkar, need more support.

References

cc_apt_configure - documentation cc_apt_configure - source code cc_apt_configure - schema config definition https://canonical-cloud-init.readthedocs-hosted.com/en/latest/development/contributing.html

[kaiwalyakoparkar]

Thank you so much @aciba90 I will start working on it and would let you'll know if I face any issue, Kindly assign this to me :D

[holmanb]

One way to implement it would be to use apt-key, but this command is deprecated, see the man page.

Preferably this would use/share existing code rather than use apt-key or direct requests calls.

If I remember correctly, we already have a function in cloudinit.util that is for file download that is currently used for pulling down user-data files over http. I would check there or in cloudinit.url_helper for existing code to use.

[kaiwalyakoparkar]

Hey @holmanb thanks for the info, where do I find functions you mentioned in cloudinit.util or cloudinit.url_helper? Can you please link it? Also @aciba90 do I only need to update the configuration file you mentioned?

[aciba90]

The schema is the place where we define what config keys are allowed per config module, and config modules receive an instance of the configuration and react appropriately to them.

In this case, we have to:

1. Extend the existing config definition for the apt config module, to hold the new required keys to support this feature.
2. Extend the apt config module to react to those keys (if present) and fetch and set up the pgp keys.

You can find the python modules here: cloudinit.util and cloudinit.url_helper.

[kaiwalyakoparkar]

Hey folks, just wanted to update you that, I might not able to work on this issue for a while due to my exams. I will resume working on it as soon as my exams are over :)

[kaiwalyakoparkar]

Folks, I am back from exams. Thanks for understanding. I will start working on this issue :)

[the-wondersmith]

Per @holmanb's comments on #4822, I'll pick this up and submit a PR as soon as I'm able

[the-wondersmith]

@aciba90 and/or @holmanb Could I get some insight / feedback on these updates to the schema?

Following @holmanb's comment, I wanted to make sure that the schema accurately reflected that certain keys conflict when present in an apt.sources.{source} entry. The new schema passes validation and the behavior appears correct, but despite being a professional python software engineer manual JSON schema creation isn't something I do regularly enough to be 100% confident I've got it right.

Also, while I was in there I took the liberty of updating the top-level properties so that the proper references are specified for each key. I assumed that they were left empty because working out each reference is just an absolute slog, but after I got it all done I reconsidered that perhaps they were intentionally left empty and couldn't find anything to confirm one way or the other. tl;dr feedback on that point would be much appreciated as well.

If the preference is that changes to that schema def are as minor as possible I can absolutely roll them back. If y'all confirm that they're correct and leaving them in won't impact the PR's acceptance though, I will obviously just leave 'em be.

[the-wondersmith]

@aciba90 @holmanb ^ bump

[holmanb]

@the-wondersmith sorry I missed your earlier comment. Could you please open a PR with your changes? Also did you sign the CLA?

It looks like you are on the right track with your work here, and your commit history looks clean - we should be able to do fast-forward merge.

I assumed that they were left empty because working out each reference is just an absolute slog

I think you're right - I expect adding them as you have would be better, but maybe @TheRealFalcon had a reason not to add them at the time. Either way, lets continue the conversation in a PR once you've opened it.

[TheRealFalcon]

perhaps they were intentionally left empty

They were. The current structure is the way it is because we generate documentation off of our schema. Originally, we didn't have a top-level properties definition at all, but that precluded us from being able to restrict top-level keys. The keys were added to fix this with blank values because they're already defined else ware in our schema in a way that still works. Adding values in the way you did won't hurt anything, but I don't think it really adds anything that will help our current schema parsing. However, it will be prone to future problems where something is updated in one list but not the other, so it's not something I want to add unless there's some value that I'm not seeing.

[the-wondersmith]

{...} Could you please open a PR with your changes?

Absolutely, will do as soon as I have some free time

Also did you sign the CLA?

Yessir, I did 😁

It looks like you are on the right track with your work here, and your commit history looks clean - we should be able to do fast-forward merge.

💪

I assumed that they were left empty because working out each reference is just an absolute slog

I think you're right - {...} Either way, lets continue the conversation in a PR once you've opened it.

Sounds good