Community
Participate
Working Groups
I'll create a Gerrit that does the following. Provide org.eclipse.equinox.p2.repository.spi.PGPPublicKeyService as a mechanism for allowing downstream frameworks to tailor how keys are located. Provide org.eclipse.equinox.internal.provisional.p2.repository.DefaultPGPPublicKeyService as the default implementation. It supports key server access, but that is disabled by default for now. It's can be enabled like this: -Dp2.keyserver=keyserver.ubuntu.com It also supports looking up keys the local GPG pubring, but that can be disabled. -Dp2.gpg=false Or can be told were to look: -Dp2.gpg=<folder-containing-pubring.{kbx|gpg} All uses of PGPSignatureVerifier.KNOWN_KEYS are replaced with this service. The PGPPublicKeyStore is modified as in Bug 578056 to use the fingerprint for uniqueness. PGPSignatureVerifier is modified to cope with multiple keys per key ID as in as in Bug 578056. It uses the PGPPublicKeyService to eliminate the static KNOWN_KEYS field. It checks for expired keys and for revoked keys; this is temporarily guarded by a system property that could be used to disable the check if it were to cause a problem. -Dp2.pgp.verifySignatureRange=false It records the PGP_SIGNATURES_PROPERTY_NAME and the corresponding PGP_SIGNER_KEYS_PROPERTY_NAME property for the verified signatures that are known to have used exactly those keys in the target artifact descriptor. This eliminates the need for special case logic in MirrorRequest.getDestinationDescriptor for these properties, which has been removed. It relies on SimpleArtifactRepository.ArtifactOutputStream implementing IAdaptable and providing access to the destination artifact descriptor via SimpleArtifactRepository.ArtifactOutputStream.getAdapter(Class<T>). This also ensures that CertificateChecker.checkCertificates knows exactly which keys were used to verify the artifact's PGP signatures that it it can prompt correctly. Note in particular that the new invariant is that for a PGP signed artifact to be successfully provisioned into the local artifact pool, not only must it have been verified for all signatures as before, but the keys used for that verification will necessarily be associated with it in the metadata of the artifact pool / artifact repository. Further refinements are planned.
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/189910
This sounds great and very flexible. I wonder if there could be more than one PGPPublicKeyService or it is expected to be a singleton? I'm asking this because these options you describe here (keyserver/pubring/...) could also been seen as a list of single PGPPublicKeyServices queried in a given order. So there could be for example 0..n KeyServerPGPPublicKeyServices, each of those backed by an OSGi config so I can use different keyservers. Again there might be 0..n PubringPGPPublicKeyServices, each of those backed by an OSGi config so I can use different locations. The we can register a default for each if a given system property is set. Then the AgentService would be some Kind of PGPPublicKeyServiceCollection collecting PGPPublicKeyServices by OSGi ordered by the service ranking and then could query each one for certain operations (e.g. searching for a key, or for a revocation...)
(In reply to Christoph Laeubrich from comment #2) > This sounds great and very flexible. I wonder if there could be more than > one PGPPublicKeyService or it is expected to be a singleton? > The Javadoc for IProvisioningAgent says this: A provisioning agent is comprised of a modular, extensible set of related services.Each agent instance has its own separate instances of these services that are not shared with other agents. There is at most one instance of a given service tracked by an agent at any given time, which ensures all services that makeup an agent instance share common service instances with each other. > I'm asking this because these options you describe here > (keyserver/pubring/...) could also been seen as a list of single > PGPPublicKeyServices queried in a given order. > A subclass of PGPPublicKeyService could support iterating over a list of such things, but... > So there could be for example 0..n KeyServerPGPPublicKeyServices, each of > those backed by an OSGi config so I can use different keyservers. > Well, then it's not a service in the sense of IProvisioningAgent.getService(Class<T>)... > Again there might be 0..n PubringPGPPublicKeyServices, each of those backed > by an OSGi config so I can use different locations. > One service could support multiple key servers, i.e., the existing "default" implementation could support a list rather than a single value. But, to be honest, I have had a hard time finding more than one server that is reliable and well behaved so at this point I'm happy if there is one that works reasonably... > The we can register a default for each if a given system property is set. > > Then the AgentService would be some Kind of PGPPublicKeyServiceCollection > collecting PGPPublicKeyServices by OSGi ordered by the service ranking and > then could query each one for certain operations (e.g. searching for a key, > or for a revocation...) There would still need to be some kind of central "service" that brings together the results from multiple key servers because having multiple different "copies" of the same keys from different servers isn't doing to work well at all. And given each key server supports the same protocol and its search results need to be handled in the same way, I don't see why we'd not just use a list of key servers. But, as mentioned above, the only well behaved server I have found is https://keyserver.ubuntu.com/. For example, keys.openpgp.org is fast, but serves up a stale key. E.e., this key says it's expired: https://keys.openpgp.org/vks/v1/by-fingerprint/3C91FED3922D52296888BAE9B6D3AB9BCC641282 keyID=0x700e4f39bc05364b fingerPrint=0x9e3044071b758ebcb7e45673700e4f39bc05364b masterKey=false encryptionKey=true algorithm=1 bitStrength=4096 creationTime=2016-12-21T10:44:11Z validSeconds=157680000 expiration=2021-12-20T10:44:11Z While this server has the key expiring in 2026: https://keyserver.ubuntu.com/pks/lookup?search=0x700E4F39BC05364B&fingerprint=on&op=index And, here I thought all the key servers sync their keys so it doesn't really matter where you ask but apparently that's not the case. Perhaps you could suggest more than one server that reliably serves up key?
I think checking key expiry is not so helpful: https://stackoverflow.com/questions/38486826/what-happens-to-openpgp-signed-git-commits-after-key-expiration This talks about issue a warning: When verifying signatures, OpenPGP implementations will compare the expiry date with the date the signature was issued. If the signature was issued within the expiration period, you're fine. If not, it will issue a warning (something like "the signature was fine, but issued after expiration). So I suppose at best we could/should log a warning but not treat it as an error...
FYI, I refactored the implementation such that one or more key servers can be specified, e.g, -Dp2.keyservers=keyserver.ubuntu.com,keys.openpgp.org This provides better key management because there now is a cache folder specific to each key server which directly caches the server's query results. The overall service combines the results from the key server(s), the locally added keys (as specified in p2 repository/artifact metadata), as well as the global GPG pubring's keys.
Created attachment 287956 [details] Testing multiple signatures on an artifact I found the original PGP signature of this artifact: https://repo1.maven.org/maven2/org/assertj/assertj-core/3.21.0/ I used that to modify the p2 artifact metadata to specify both that signature and the platform's signature. With key servers enabled, p2 is able to find that signature's key, i.e., this: https://keyserver.ubuntu.com/pks/lookup?search=0xBE685132AFD2740D9095F9040CC0B712FEE75827&fingerprint=on&op=index And it was able to verify that both keys signed the artifact. This is presented to the user like the attached (with fake missing 509 cacerts). Only one of the two PGP keys needs to be trusted, but of course the user cannot know that this is the case.
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/189910 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=98957cf13269d710fba4a1c02a7bdc4c36a3c015
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.common/+/190352
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.common/+/190352 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/?id=809a07a73065664d78f787a14cfb3c2c58f91afd
Created attachment 287983 [details] problems with trust dialog I see some issues with the new "PGP trust" dialog. I've attached screenshot with my comments. 1) The warning that is *written into the column title*. Sorry, no way, this is not how a warning should be added to a wizard / dialog. Beside that it is highly unusual, it fully hides the next "validity" column. I would propose to add only an icon to the column and a tooltip over it. Optionally, this warning could be added to the top of the wizard. 2) Even as experienced Eclipse user I have NO CLUE which bundle (or bundles) use this PGP key. Without this context I have no clue if the all 100 bundles I'm going to install are "not to be trusted" or just a few or a single one. IMHO this is crucial to know. There must be some info shown which bundles are using this PGP key. 3) I have -Declipse.p2.unsignedPolicy=allow set, but PGP trust dialog is appearing nevertheless. I wish I could have a system flag (or we just re-use the old one) that skips this PGP trust dialog altogether, simply because I don't want to explain every developer in the lab why to the hell we have some PGP keys for the software we install locally from our internal update site.
> 2) Even as experienced Eclipse user I have NO CLUE which bundle (or bundles) > use this PGP key. Without this context I have no clue if the all 100 bundles > I'm going to install are "not to be trusted" or just a few or a single one. > IMHO this is crucial to know. There must be some info shown which bundles > are using this PGP key. It's equivalent to how unrooted x509 signature trust question has been presented for a long time. I don't think we should aim for something more advanced here and disrupt too much the existing review processes at this stage. Currently people who have a doubt about a key or a certificate need to review their installation without the dialog. Improving that would be a separate topic, if there is not already a request existing about it for certificates that could as well apply to PGP keys.
(In reply to Mickael Istria from comment #11) > > 2) Even as experienced Eclipse user I have NO CLUE which bundle (or bundles) > > use this PGP key. Without this context I have no clue if the all 100 bundles > > I'm going to install are "not to be trusted" or just a few or a single one. > > IMHO this is crucial to know. There must be some info shown which bundles > > are using this PGP key. > > It's equivalent to how unrooted x509 signature trust question has been > presented for a long time. Which I've never seen, differently to the new one. Also pointing to a usability issue in existing code is not a reason to repeat same error in the new code. One should aim to provide a *better* solution, not a worse. > I don't think we should aim for something more > advanced here and disrupt too much the existing review processes at this > stage. It is not advanced, it is basic. Other way around: PGP key dialog without any context IS advanced BECAUSE there is no context. It appears out of nowhere, gives little or nothing explanation and asks questions that can't be answered without additional info. > Currently people who have a doubt about a key or a certificate need to > review their installation without the dialog. People don't doubt. They have an internal update site and trust that, but if such dialogs appear during installation procedure, an avarage *responsible* and *security sensible* user will for sure ask - may be there is some attack or I'm doing something wrong. The average user has no chance to put this into the context at all. I myself need a debugger to understand where this key is coming from and what is the consequence of approving that dialog. > Improving that would be a > separate topic, if there is not already a request existing about it for > certificates that could as well apply to PGP keys. I can't understand why an improvement to the dialog that is explicitly implemented for *this* ticket is a separate topic - the dialog need to be improved, it is not OK as it is. If the request to improve this dialog should go to a different topic, I would not show that dialog at all *by default*, because it is more confusing as useful today. Log some message to the log ("installed something with PGP key "XYZ", no idea if that is trusted or not, check if you like") and the users will be happy.
(In reply to Andrey Loskutov from comment #12) > Which I've never seen, differently to the new one. The dialog is not supposed to be popping up so often in the end as many keys would be trusted by default in the IDE. It's a transitional state. > Also pointing to a usability issue in existing code is not a reason to > repeat same error in the new code. One should aim to provide a *better* > solution, not a worse. It's the same code, the same trust dialog. So it's not a new issue and it's not in the scope of PGP support at the moment. > It is not advanced, it is basic. > Other way around: PGP key dialog without any context IS advanced BECAUSE > there is no context. It appears out of nowhere, gives little or nothing > explanation and asks questions that can't be answered without additional > info. OK, but nothing new here. Nothing that is a must-have for PGP comparing it to x509. > I can't understand why an improvement to the dialog that is explicitly > implemented for *this* ticket The dialog has always been here. It's just been extended to show PGP keys as a new source, additionally to x509, for this ticket. What the dialog shows and how it's supposed to be used is no different. But contributions are for sure welcome to make this dialog richer.
Note that the usability issues you raise are related to the following bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=578024 This bug did not introduce the dialog, but just provided some improvements. It's still sadly lacking, but the topic here was support for key servers such that web of trust can be displayed. Please record your legitimate concerns about the dialog in 578024 so they're all in one place. There's also the following bug complaining about not knowing which bundles are using the certificate: https://bugs.eclipse.org/bugs/show_bug.cgi?id=541347 This now also applies knowing which bundles use the PGP key, and that is made worse because as you can see in the first attachment here, that a bundle could be signed by more than one key, and you only need to trust one, but not both. How to make that usable? A system property for a policy like there is for signing would also be good. Please open a specific Bugzilla for that.
(In reply to Mickael Istria from comment #13) > The dialog has always been here. It's just been extended to show PGP keys as > a new source, additionally to x509, for this ticket. What the dialog shows > and how it's supposed to be used is no different. Well, adding a new feature (PGP) by re-using old dialog that was not usable is one way to say "it was always here". From the user perspective (I'm the user), I've never seen this dialog, not it is there and it is ugly. So it is a clear regression in 4.23, caused by the PGP changes. > But contributions are for sure welcome to make this dialog richer. If you do not plan to work on the usability of this dialog, I would propose to not show this dialog by default. *This* will improve the usability and avoid regression. OK, I will push a patch that avoids opening this dialog if the "-Declipse.p2.unsignedPolicy=allow" system property is set, similar what is done in org.eclipse.equinox.internal.p2.engine.phases.CertificateChecker.getUnsignedContentPolicy()
(In reply to Andrey Loskutov from comment #15) > (In reply to Mickael Istria from comment #13) > > The dialog has always been here. It's just been extended to show PGP keys as > > a new source, additionally to x509, for this ticket. What the dialog shows > > and how it's supposed to be used is no different. > > Well, adding a new feature (PGP) by re-using old dialog that was not usable > is one way to say "it was always here". > > From the user perspective (I'm the user), I've never seen this dialog, not > it is there and it is ugly. So it is a clear regression in 4.23, caused by > the PGP changes. > > > But contributions are for sure welcome to make this dialog richer. > > If you do not plan to work on the usability of this dialog, I would propose > to not show this dialog by default. *This* will improve the usability and > avoid regression. > I do plan to work on this and note that this dialog was there before 4.23, you just didn't see it... > OK, I will push a patch that avoids opening this dialog if the > "-Declipse.p2.unsignedPolicy=allow" system property is set, similar what is > done in > org.eclipse.equinox.internal.p2.engine.phases.CertificateChecker. > getUnsignedContentPolicy() I think it would be better to have a different flag with similar behavior as for unsigned but applying only to PGP signatures.
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/190442
(In reply to Ed Merks from comment #14) > Note that the usability issues you raise are related to the following bug: > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=578024 > Please record your legitimate concerns about the dialog in 578024 so they're > all in one place. Done. > A system property for a policy like there is for signing would also be good. > Please open a specific Bugzilla for that. Not sure if this is required. If user says "allow unsigned bundles", why he should be prompted for bundles that *signed* but with unknown key? So for my use case, where "-Declipse.p2.unsignedPolicy=allow" is already set, there is no additional use case of a second property saying basically that "I don't care". Other way around: if someone wants to have *only* signed bundles (and cares about signed/unsigned differences), why this someone would request "but please don't show me dialog for unknown PGP keys". Either you have your security under control and don't care about extra dialogs, or you don't trust anyone and wants explicit checks for everything. Germans say "man kann nicht ein bisschen schwanger sein" - "you cannot be just a little pregnant" - with security it is same.
(In reply to Andrey Loskutov from comment #18) > > A system property for a policy like there is for signing would also be good. > > Please open a specific Bugzilla for that. > > Not sure if this is required. > > If user says "allow unsigned bundles", why he should be prompted for > bundles that *signed* but with unknown key? So for my use case, where > "-Declipse.p2.unsignedPolicy=allow" is already set, there is no additional > use case of a second property saying basically that "I don't care". > > Other way around: if someone wants to have *only* signed bundles (and cares > about signed/unsigned differences), why this someone would request "but > please don't show me dialog for unknown PGP keys". Either you have your > security under control and don't care about extra dialogs, or you don't > trust anyone and wants explicit checks for everything. Germans say "man kann > nicht ein bisschen schwanger sein" - "you cannot be just a little pregnant" > - with security it is same. Please can we have this discussion in a bug dedicated to that theme? Please please... I really don't feel it's appropriate to change the meaning of this flag to also apply to certificates now too.
(In reply to Ed Merks from comment #19) > Please can we have this discussion in a bug dedicated to that theme? Please > please... I really don't feel it's appropriate to change the meaning of > this flag to also apply to certificates now too. See bug 578583
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/190804
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/190804 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=2b6d277e425dab6d0a07d0422274e5dce4222813
Further improvements, such as a UI to control they key servers, are deferred to the future via a separate enhancement request.
