[FEATURE REQUEST] pkgrepo should follow current Debian standards

[OrangeDog]

Is your feature request related to a problem? Please describe.
Debian have deprecated apt-key and strongly recommended no longer adding third party keys to the global apt trust.
Instead, these recommendations should be followed: https://wiki.debian.org/DebianRepository/UseThirdParty

The key MUST be downloaded over a secure mechanism like HTTPS to a location only writable by root, which SHOULD be /usr/share/keyrings. The key MUST NOT be placed in /etc/apt/trusted.gpg.d or loaded by apt-key add.

A sources.list entry SHOULD have the signed-by option set. The signed-by entry MUST point to a file, and not a fingerprint.

Note that the name of the key file NAME-archive-keyring.gpg and sources list NAME.list should match.

Describe the solution you'd like
When the pkgrepo.managed state should be able (and ideally prefer) to set up repos according to these specifications.

deb [arch=amd64 signed-by=/usr/share/keyrings/salt-archive-keyring.gpg] http://repo.saltproject.io/py3/debian/10/amd64/latest buster main:
  pkrepo.managed:
    - file: /etc/apt/sources.list.d/salt.list
    - key_url: https://repo.saltproject.io/py3/ubuntu/20.04/amd64/latest/salt-archive-keyring.gpg

The same should be done when the key is from a keyserver. The file may also require de-armouring if the repo isn't following the new specification.

To make this easier, the current (inconsistent with other backends) behaviour of requiring the name to be the verbatim line may need deprecating or otherwise avoiding. For standards-compliant repos it should be as simple as possible, with file, key_url, comps using the defaults.

salt-repo:
  pkgrepo.managed:
    - architectures: amd64
    - baseurl: http://repo.saltproject.io/py3/debian/10/amd64/latest  # would ideally default to name
    - dist: {{ grains["oscodename"] }}

Describe alternatives you've considered
Doing it manually with file.managed states and ignoring pkgrepo.

Additional context
Salt itself echoes this specification in its instructions: https://repo.saltproject.io/#debian
It's a bit paradoxical that it doesn't have the ability to follow it itself.

[OrangeDog]

Just noting that an armoured keyring (e.g. /usr/share/keyrings/salt-archive-keyring.asc) does also work, though as the standard says:

The file SHOULD NOT be ascii-armored

[OrangeDog]

@sagetherage the label says

Related to packaging of Salt, not Salt's support for package management.

[sagetherage]

true, I likely need a better label, ATM I am not sure what that would be, exactly. perhaps 'package-repo"?

[sagetherage]

moving this out of triage as we will need to follow a standard and for debian packages at least we should follow their standard, this will need to be planned, but moving from triage to planning.

[bryceml]

Removed from the Packaging project since this is about salt code, and not salt's Packages or Repos directly.

[mherrb]

Are there working ways to handle repositories with a separate keyking (signed-by=) in the mean time ?

Currently for me, when trying to manually add a [signed-by= ] tag to the name attribute, salt keeps adding a new line to the .list file each time it's called.

[OrangeDog]

@mherrb

Doing it manually with file.managed states and ignoring pkgrepo.

[udf2457]

Do you think its worth adding a mild warning note to the docs ? Just to perhaps point people in the right direction about how to do it properly ? (e.g. present a workaround snippet in the docs ?)

[eliasp]

Just noting that an armoured keyring (e.g. /usr/share/keyrings/salt-archive-keyring.asc) does also work, though as the standard says:

The file SHOULD NOT be ascii-armored

This refers only to the *.gpg-file, which should be a GnuPG binary key, while *.asc should be an armored file.
Although the docs on https://repo.saltproject.io/#ubuntu point now towards the binary file, I lean towards defaulting to .asc/armored which is "human readable".

[udf2457]

@eliasp it clearly says at the top of the first paragraph on the page (https://wiki.debian.org/DebianRepository/UseThirdParty):

A binary export (gpg --export) of the key SHOULD be available at the root of the repository under the filename deriv-archive-keyring.gpg, where deriv is the a short name for the repository. The file SHOULD NOT be ascii-armored (gpg --export --armor) although a separate armored version MAY be available under deriv-archive-keyring.asc.

i.e. the default SHOULD be binary and you CAN provide ascii in addition, NOT instead !

I would also point you to the box on that page that highlights the following statement:

The reason why we avoid ASCII-armored files is that they cannot be used directly by SecureApt

[eliasp]

Ok, you convinced me… binary it is then. 😄

[dhs-rec]

According to that man-page, which also clearly says (right at the top) that this utility is deprecated, neither /usr/share/keyrings, nor /etc/apt/keyrings should be used, but /etc/apt/trusted.gpg.d:

/etc/apt/trusted.gpg.d/
File fragments for the trusted keys, additional keyrings can be stored here (by other packages or the administrator). Configuration Item Dir::Etc::TrustedParts.

Which is (again) in contrast to the wiki page, which says:

The key MUST NOT be placed in /etc/apt/trusted.gpg.d or loaded by apt-key add.

The question now is whether to follow the man-page of a deprecated tool or rather the Debian guidelines...

[udf2457]

I already told you previously that you linked to the testing release man page, not the bullseye manpage.

You should be following the man page in the release of the operating system that you are using, i.e. bullseye man page for the bullseye release.

[dhs-rec]

But now I was referring to the bullseye man-page, as linked by you: https://manpages.debian.org/bullseye/apt/apt-key.8.en.html

apt-key - Deprecated APT key management utility

[OrangeDog]

Nobody needs to read any manuals or anything to find where the key is, because it's listed right in sources list with signed-by.

Just make it an optional parameter, and default it to /etc/apt/keyrings. Then add another to choose whether to create it (with correct permissions) if it's missing, or require the user to put a file.directory themselves.

[Ch3LL]

I merged in #61760 but I will follow up next week with another PR with these changes.

[Ch3LL]

K thoughts on this? #62159

[dhs-rec]

It's still not quite correct, sorry. It says (in pkgrepo.py):

The recommended way to manage repo keys going forward is to download the keys into /etc/apt/keyrings...

But, again, this is only half of the truth. The wiki page states (see above):

If future updates to the key will be managed by an apt/dpkg package as recommended below, then it SHOULD be downloaded into /usr/share/keyrings using the same filename that will be provided by the package. If it will be managed locally, it SHOULD be downloaded into /etc/apt/keyrings instead.

So the destination directory (and filename) really depends on how the specific key will be managed after its initial creation...

[Ch3LL]

Since we are using the pkgrepo state to manage the key and not a package it would be /etc/apt/keyrings then. Thats how I understand that statement.

[OrangeDog]

@dhs-rec there's no possible way for Salt to know the future plans of some other developer. If the user happens to know that it will be package-managed in future, then they can pass /usr/share/keyrings to the state. The default should be /etc/apt/keyrings.

[heini]

But the person writing the Salt state knows. There are projects out there which provide keyring packages. In this case one would simply:

foo:
  pkgrepo.managed:
    - humanname: Foo Package Repository
      ...
      require_in:
        - pkg: foo
        - pkg: foo-keyring
  pkg.installed:
    - version: latest
foo-keyring:
  pkg.installed:
    - version: latest

In this case the key should go into /usr/share/keyrings, otherwise into /etc/apt/keyrings.

[Ch3LL]

As @OrangeDog stated you would need to pass in the keydir path. If you are using the pkgrepo.managed state the signedby argument needs to be passed in via the name of the state or the signedby state arg or else the repo will not know where the key is.

[heini]

Yes, sure.

[Ch3LL]

Closing now that #62159 is merged

[OrangeDog]

pass in the keydir path

the signedby state arg

Neither of these have been documented.

[Ch3LL]

The signedby state arg was removed here: #62215 with an explanation in the PR.
The keydir argument is documented here: https://docs.saltproject.io/en/latest/ref/modules/all/salt.modules.aptpkg.html#salt.modules.aptpkg.add_repo_key and on all of the methods that accept that argument.

[OrangeDog]

Oh I see. Thought it was going to be on the state function, given the previous discussion.

[udf2457]

I'm with @OrangeDog . Bit of a weird decision.

[heini]

I wonder whether it would make sense to simply write the files in deb822 format (.sources instead of .list), which would allow for a more 1:1 like relation between options of this resource and those in the resulting file. For example:

deb http://deb.debian.org/debian bullseye main contrib non-free
deb-src http://deb.debian.org/debian bullseye main contrib non-free
deb http://deb.debian.org/debian bullseye-updates main contrib non-free
deb-src http://deb.debian.org/debian bullseye-updates main contrib non-free

would become

Types: deb deb-src
URIs: http://deb.debian.org/debian
Suites: bullseye bullseye-updates
Components: main contrib non-free

Options in [], like signed-by or arch simply become an additional entry, like

Architectures: amd64 arm64
Signed-By: /path/to/keyring

See sources.list(5) for the details.

[OrangeDog]

It should also be possible to support both formats without breaking anything. For example, if the name starts with deb or if file ends with .list then use list format, otherwise use sources format.

[jdelic]

hmm, I would prefer an explicit flag (defaulting to whatever fits backwards compatibility) over magic any day.