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

[BUG] 2023 GPG key name does not follow Debian standards #64144

Closed
OrangeDog opened this issue Apr 24, 2023 · 27 comments · Fixed by saltstack/salt-install-guide#7
Closed

[BUG] 2023 GPG key name does not follow Debian standards #64144

OrangeDog opened this issue Apr 24, 2023 · 27 comments · Fixed by saltstack/salt-install-guide#7
Assignees
Labels
Bug broken, incorrect, or confusing behavior debian affects this operating system Packaging Related to packaging of Salt, not Salt's support for package management.

Comments

@OrangeDog
Copy link
Contributor

OrangeDog commented Apr 24, 2023

Description

The GPG key for the 3006.0 release is now named: SALT-PROJECT-GPG-PUBKEY-2023

Expected behavior
As per https://wiki.debian.org/DebianRepository/UseThirdParty it should be called salt-archive-keyring, as it was previously.

@OrangeDog OrangeDog added Bug broken, incorrect, or confusing behavior Packaging Related to packaging of Salt, not Salt's support for package management. Regression The issue is a bug that breaks functionality known to work in previous releases. needs-triage debian affects this operating system labels Apr 24, 2023
@bensteinberg
Copy link
Contributor

See also #64133

@whytewolf
Copy link
Collaborator

so, I will disagree that it doesn't follow standards. SHOULD != MUST and we do follow all of the MUSTS. they are very careful to capitalize important words in their documentation about what is a must vs what is a should. as it is should and not a must as long as we documented the difference which we did we are still within standard.

@dmurphy18
Copy link
Contributor

@OrangeDog There actually was an issue (not finding it at the moment) which resulted in using SHA-256 salt-archive-keyring-2023.gpg so that the use of the old SHA-1 key in salt-archive-keyring.gpg was not over-written and allow for quick reversion etc.

@OrangeDog
Copy link
Contributor Author

You still really should do all the SHOULDs. It makes it easier for everyone.

@dmurphy18
Copy link
Contributor

dmurphy18 commented Apr 26, 2023

See closed issue #63953 where the issue was considered, and made a documentation issue.

It will be discussed in today's team meeting

@OrangeDog
Copy link
Contributor Author

OrangeDog commented Apr 26, 2023

this allows users to quickly switch between different versions of Salt without overwriting the keyring

The solution to that was #63925.
Regardless, you changed the name of the source file instead of the destination, so it's still overwritten and doesn't solve the problem.
The file name in the repo should stay the same.

it should not append but overwrite the existing salt.list file

That's also causing a lot of pain. It 100% should be appending.

@dmurphy18
Copy link
Contributor

@OrangeDog We are discussing the documentation too, it was supposed to change. and #63925 was closed as not going to do that.

@rittycat
Copy link
Contributor

I would just like to add that I've been having a bunch of issues getting things to work right again after this change due to certain keys being in some places, but not others, and having to make sure I'm using the right one.

I don't understand why doing this change in this way was so necessary.

@dmurphy18
Copy link
Contributor

The changes in the key used for signing was necessary for the following reasons:

  • RHEL 9 dropped support for SHA-1 keys and now needs SHA-256 (3005.1 had a new SHA-256 key)
  • Ubuntu 22.04 dropped support for SHA-1 keys and now needs SHA-256 (3005.1 had a new SHA-256 key)
  • Fedora 39 is moving to using SHA-256 keys
  • Currently supported OS are capable of using SHA-256 keys

With the release of a major version in 3006 and a new backend with relenv, it was decided that now was the time to switch to using a SHA-256 key for all Salt supported OS's, especially with the new tool chain using GitHub Actions.

Openssl v3.x drops support for SHA-1, unless special efforts are made to support older SHA-1 keys, this is due to SHA-1 not being safe anymore for cryptographic work. RHEL 9 and Ubuntu 22.04 support Openssl v3.x.

Betting Debian 12 when released in June will also be similar in using SHA-256 too.

@OrangeDog
Copy link
Contributor Author

None of that explains why the file name was changed.

@dmurphy18
Copy link
Contributor

@OrangeDog I believe we are in a circular argument, the new name is to identify the SHA-256 version of the key, and to allow users to keep the old SHA-1 key around. I understand that you feel that they should be combined, however keeping a possible suspect SHA-1 key around is not conductive to security moving forward. Best to have it easily identifiable and removed when it is no longer required.

SHA-1 is no longer safe. see #63953 for part of this discussion
The triggering for that issue was a Community Slack comment about over-writing the file and preventing the person from switching back and forth, between existing and the RC.

@OrangeDog
Copy link
Contributor Author

OrangeDog commented Apr 28, 2023

That's not what I'm talking about. There is no need to identify the SHA-256 version of the key, because you aren't including two key files in the same repo. You just need to identify the key for that repo, which is why it should have a standardised name.

The trigger for this issue was not being able to find the key for the new repos, because the name was changed for no reason.

@dmurphy18
Copy link
Contributor

So the old SHA-1 key was called SALTSTACK-GPG-KEY.pub, the SHA-256 key used in Salt 3005.1 for RHEL 9 and Ubuntu 22.04 was called SALTSTACK-GPG-KEY2.pub.

But Salt is not SaltStack anymore, and it is now Salt Project, hence the new SHA-256 key is called SALT-PROJECT-GPG-PUBKEY-2023.pub, the year is used to help track when it was created.

The old SALTSTACK-GPG-KEY.pub has been in use for almost a decade, without it being compromised (lucky or careful), but cannot guarantee that the names for keys won't change in the future, if there is a security breach, which appears more and more through no fault, for example: recent large effort redoing keys due to LastPass breach, fortunately Salt signing key was unaffected, but that was luck.

The key is used across a number of repositories and OS's, and the change was clearly marked and adjusted in the RC notes, due to feedback from the Community, so as to not affect current keys held.

Understand your dissatisfaction, but the changes are reasonable.

@OrangeDog
Copy link
Contributor Author

Salt is not SaltStack anymore

Then why does 3006 install to /opt/saltstack?

so as to not affect current keys held.

Then why do the instructions specifically overwrite the current key?

@OrangeDog
Copy link
Contributor Author

OrangeDog commented Apr 28, 2023

It's very simple. The keyring SHOULD be at https://repo.saltproject.io/salt/py3/ubuntu/22.04/amd64/salt-archive-keyring.gpg.

No reason has been given as to why it isn't. It can't conflict with anything, because there are no other keyrings in that folder.

I apologise that I mistakenly thought it was called that before. I thought that had been done during the last round of standards-meeting. It would have been better to point that out earlier.

@OrangeDog OrangeDog removed the Regression The issue is a bug that breaks functionality known to work in previous releases. label Apr 28, 2023
@anilsil anilsil added this to the Sulfur v3006.2 milestone May 10, 2023
@lmf-mx
Copy link
Contributor

lmf-mx commented Jun 13, 2023

While the distinction between hashes mentioned is understandable, it is not simply an issue with expectations around best or most preferred practices, but rather one around templating and automation.

  • How will we determine the filepath to the key automatically when a release in the year 2024 happens?
  • How will we determine the filepath to the key for a release that was released prior to 2024?

Example: If I need to install 3006 in 2024, how do I, in code, determine to use the key SALT-PROJECT-GPG-PUBKEY-2023.gpg? If I also need to install a version released in 2024, how do I determine the key's name?

The fact is this is a platform for simplifying, templating, and automating execution and this introduces a blocker.

@dmurphy18
Copy link
Contributor

dmurphy18 commented Jun 13, 2023

@lmf-mx The naming of the key merely reflects the year it was created, hopefully there will be no need for a new key in 2024. Noting the old SHA-1 key was around for many years. Having 2023 in the name for the new key enables searches for what timeframe was the key created when / if, we have to ever revisit decisions as to why the key was generated in the future. It also allowed for it to co-exist with the old key in the same directory, and leave it singular. Combining SHA-1 and SHA-256 keys together is a bad idea IMHO, given SHA-1 is already cracked and support for it already removed in some distributions with other about to remove support for, e.g. Fedora for one.

As for noting following Debian standards, SALTSTACK-GPG-KEY.pub was used for years without complaint, and only recently changes to salt-archive-keyring, same issue with the '-2023', allow for singular.

@OrangeDog
Copy link
Contributor Author

allowed for it to co-exist with the old key in the same directory

Except it never did. No repo has both of them. The name of it in the repo is completely unrelated to the path you download it to.

@lmf-mx
Copy link
Contributor

lmf-mx commented Jun 13, 2023

@dmurphy18 Thanks for the additional detail on expectations going forward with changes. I couldn't find any documentation of the naming change and whether to expect a new version annually.

From what I could find, as mentioned by @OrangeDog, only one key exists in any directory. This is no longer a simple constant in https://github.com/saltstack-formulas/salt-formula/blob/master/salt/osmap.yaml. Now, there must be a mapping and/or logic to determine which filename to use based on other factors, such as version. Forward looking, this adds overhead that is unnecessary. Any normal box should be updating the keyring to the new key once it switches to a newer repo.

I understand the human implications in visually identifying at a moment's notice the difference in keys. I think allowing for consistent templating should be a bigger concern. The damage is already done until the repository's contents age out from being served. I would still suggest considering future updates to not create new key paths unnecessarily. If I'm missing something in understanding the benefits, I'd love to understand what it is.

@dmurphy18
Copy link
Contributor

@lmf-mx The need for a single key was recognized, and that is why from Salt 3006.x on, there is only a single key. Fortunately the older OS's which could not handle a SHA-256 had EOL'd. During RC testing, there was a need for both to exist as users complained about the over-writing etc, hence user demand during RC testing is part of the reason for the new key and naming too. Once we had to have two keys available, it made sense to do it by year, as opposed to another digit, that is a SALTSTACK-GPG-KEY3.pub would have raised questions about SHA3-xxxx.

Note that this was also an issue with Salt 3005.1 where both RHEL 9 and Ubuntu 22.04 did not support SHA-1 keys and had to create and use SALTSTACK-GPG-KEY2.pub etc.

This could change in the future too to support SHA-512, given the recent scare about a MIT student cracking SHA-256, he didn't upon review, but had a worried nights sleep out of it :) .

@dwoz
Copy link
Contributor

dwoz commented Jun 18, 2024

@ScriptAutomate @dmurphy18 Let's discuss this and try to come to a resolution on a path forward (or not).

@dmurphy18
Copy link
Contributor

I will bring a set of stakes to kill this completely, it is like a vampire raising up. 🤣
Wonders what the RedHat standard is ?

@dmurphy18
Copy link
Contributor

dmurphy18 commented Jul 22, 2024

My thoughts are to clean up and do the https://repo.saltproject.io/salt/py3/ubuntu/22.04/amd64/salt-archive-keyring.pgp for Salt 3008 (as per @OrangeDog ), given 3008 will be ground changing with the salt extensions etc., and classic packaging will have been long in the rear-view mirror at that stage.

Still need to check into RedHat family key signing, such that similar workflow occurs.

Have not played with formulas in over a decade, wonder how they will distinguish between handling for Debian/Ubuntu vs RedHat families, never mind other OS's too.

Need to check that https://repo.saltproject.io/salt/py3/ubuntu/22.04/amd64/salt-archive-keyring.pgp, ending in pgp ?, didn't think pgp was still a thing. Will reread the Debian guidelines.

Also @lmf-mx the https://github.com/saltstack-formulas/salt-formula/blob/master/salt/osmap.yaml, appears to need updating, given classic packages are EOL, and wondering about lines such as https://github.com/saltstack-formulas/salt-formula/blob/570c44b1666d356eac0cf0c7976f221056ead460/salt/osmap.yaml#L19, Python 2 has been dead for some time.

@dmurphy18
Copy link
Contributor

The Debian instructions https://wiki.debian.org/DebianRepository/UseThirdParty look like they need revision, since they are targeting Stretch (Debian 9 - EOL), and current is Bookworm (Debian 12) and mentions apt-key which is deprecated.

Will look for later instructions given these are 3 major versions behind.

@dwoz dwoz modified the milestones: Sulfur v3006.9, Sulfur v3006.10 Jul 24, 2024
@OrangeDog
Copy link
Contributor Author

There's also no reason you cannot have multiple keys in a keyring file, even if some of them are unsupported and/or expired.

@dmurphy18
Copy link
Contributor

dmurphy18 commented Oct 8, 2024

@OrangeDog PITA to deal with, am very much Unix weeny, one tool / one job / one task, also easier to manage, old vs new etc.
Understand Debian way to do things, but there is what is convenient for Salt too, don't see all the old releases in the repo either, testing PITA and by limiting the repo to one release, ensures no cross contamination between various OS releases, and makes testing easier.
Just because you can, does not mean you should. And if the argument is that is what Debian does, then let me introduce you to IBM and JCL, (insane way to control jobs on IBM 360's etc, vs. Digital DEC-20).
Every OS has its share of 'why did they do it that way' 🙄 🤣

Planning on addressing this in the 3008 timeframe, what with lots going to salt-extensions, etc.

@OrangeDog
Copy link
Contributor Author

It's not easier to manage though, as evidenced by all the complaints on this thread.

Not just because you can, but because the standard says you SHOULD and it makes it easier for everyone.

You certainly should not be deliberately going against standard practice on supported OSs just because you think you know better. Salt's apt support, both the states and its repo layout, are a big enough pain as it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior debian affects this operating system Packaging Related to packaging of Salt, not Salt's support for package management.
Projects
None yet