A current bug is that the pkgrepo state for ubuntu/debian will not update the gpg key unless some other change is made to the APT source itself. This is due to the comparison method for determining if a repo needs to be updated or not.
Oops this is a bit premature, this is assuming #4437 gets merged. If #4437 is not merged, this can be ignored.
Yes, I would assume that we will merge #4437 so this looks good, thanks!
@morganfainberg can we get an update with more info on this? I'm not entirely sure if this is the issue I'm running into or not. My symptom is that states with a keyed ppa pkgrepo need to be run twice before they succeed.
@jesusaurus the specific problem introduced with the latest pull requests around apt repo management is that the keys themselves will not be updated unless another change to the repo def occurs. this is because there was not an easy way to do introspection on the keys in short order (compare the key to another key). I'm sure it can be solved easily, but for the timeline of the last pull-req it didn't make sense.
I don't know why you're having to run the state twice. It might be an order of operation with adding the keys. I'd need more information to dig into it further. A simple example SLS should be enough to get further into it (from what I recall key'd repos worked locally for me; however, it has been a while since I submitted the pull-reqs and I haven't had a chance to circle back to this issue yet due to day-job requirements).
I've started a fix for this, but don't have the time to finish it right now. I'm using the following workaround:
base:
pkgrepo.managed:
- name: deb file:/my/repo {{ grains['oscodename'] }} my-extra
- file: /etc/apt/sources.list.d/myextra.list
- key_url: file:///my/repo/conf/[email protected]
cmd.run:
- name: apt-key add /my/repo/conf/[email protected]
- unless: apt-key list | grep -q me@my
It's still an issue, I try to dig into the code and maybe there is a solution but it's quite dirty:
In pkgrepo state, it call expand_repo_def in order to be able to check if repository need updating : https://github.com/saltstack/salt/blob/develop/salt/states/pkgrepo.py#L232
Then it compares each key returned by expand_repo_def to kwargs (https://github.com/saltstack/salt/blob/develop/salt/states/pkgrepo.py#L255).
The expand_repo_def for apt based systems is located here: https://github.com/saltstack/salt/blob/develop/salt/modules/aptpkg.py#L1349 and returned these keys https://github.com/saltstack/salt/blob/develop/salt/modules/aptpkg.py#L1390-L1397
The returned dictionary doesn't include a "keyid" value while it receives the value from the state.
We could use theses lines (https://github.com/saltstack/salt/blob/develop/salt/modules/aptpkg.py#L1194-L1198) to check if corresponding key was already imported or not. If it was imported, return keyid else return None.
If keyid is None in the dictionnary returned by expand_repo_def the state will detect that it need to update the repo and will call mod_repo which will import the gpg key.
But it will not works with key_url.
If it's a good solution, I will make a pull-request with the changes.
I tried in local env with theses changes: https://github.com/novapost/salt/compare/4438?expand=1 and it seems to works. Need some tests now...
One question, why do we pass kwargs in 'apt-key export' cmd call (https://github.com/saltstack/salt/blob/develop/salt/modules/aptpkg.py#L1196)?
This is definitely still an issue, and the workaround is a bit on the ugly side. I've not looked too deeply into the mechanics of how the pkgrepo module manages keys yet. Any chance this will get some attention or does it require some significant work to fix?
After almost 4 years this issue is still open.
As a workaround I manually removed the repo. After salt re adds the repo the new key is installed as well.
Another workaround is to have a comments field in your pkgrepo state and change that when you change the key. Salt detects the change to comments and updates the repository, using the new key.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.
Bug still exists.
Thank you for updating this issue. It is no longer marked as stale.
Hey, I was trying to look into this... but I'm having a heck of a time finding a sample state that works here without you know, creating my own PPA 馃槤
Does anyone have a state using a publicly available PPA that I can use to repro/test this?
Seems like we were using the Nginx and Rabbitmq PPAs when we hit this issue initially. For testing, I'd say you will need to setup a tmp PPA so you can test rotating the keys.
Oh yeah, this is definitely wonky. Easiest way to reproduce. Assume you have a ppa, I built a simple one using reprepro and use this state:
this is broken:
pkgrepo.managed:
- humanname: Test server
- name: deb http://dev:55555/ stretch main
- gpgcheck: 1
Apply the state. Then change it:
this is broken:
pkgrepo.managed:
- humanname: Test server
- name: deb http://dev:55555/ stretch main
- gpgcheck: 1
- key_url: http://dev:55555/keys/test.gpg.key
this fails:
pkg.installed:
- name: mypkg
Installing that package fails with E: There were unauthenticated packages and -y was used without --allow-unauthenticated.
I'm not sure what the right way is to check that the key listed is actually set and accepted. I'll take a look into this code and see if I can figure that out.
After a cursory glance at the code my theory is that it might have something to do with the file cache. In my specific case, at least, in the aptpkg module, key_url is only in the mod_repo function.
Ooooh. No, wait, now that I think about it, I bet that it's in https://github.com/saltstack/salt/blob/develop/salt/states/pkgrepo.py#L419,L475 - there's probably nowhere in there that it's checking the key_url to determine if that needs any updates, so it doesn't actually apply the updates.
I believe I've got a pretty solid handle on this - we've got a new hire starting in a week or two, and I think this will be a rather nice intro task for them, so expect to see a PR referencing this soon!
@msummers42 could you take a look at the fix in #53537 to see if that solves it for you?
Given the lack of negative response, I'm going to go ahead and close this issue.
If anyone finds that this is still an issue, please let us know and we'll re-open it!
@waynew The changes in #53537 look great. Sorry I didn't test this when you were asking earlier in the year!