Community
Participate
Working Groups
Created attachment 287986 [details] example of the Trust PGP dialog Coming from bug 578322 comment 18. The "Trust" PGP key dialog (see bug 578024 and bug 541347) is just confusing and scaring an "average" user. There should be a possibility to avoid showing this dialog, similar to -Declipse.p2.unsignedPolicy=allow system property that was used before. I personally think this extra property is not needed, but I was asked to create a bug for that extra property if I want to hide PGP, so here it is. ### For the record: I think that an extra flag is not needed if -Declipse.p2.unsignedPolicy=allow is already set. 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.
It's a good point that if you (the users) don't care about unsigned content then you probably don't care about unrooted certificates nor about PGP trust. So what you're suggesting is not unreasonable. I'm just uncomfortable that is sudden applies to jar signed content with unrooted certificates, although again our argument is reasonable for that case as well... But in any case, I think the certificate checker needs to do the work because it shouldn't call out to the UI services and then suddenly the UI services needs to know about the policy and implement that policy...
Do you think anyone might want to allow PGP signed content for unknown keys (and unrooted certificates) without prompting while still being prompted for unsigned content?
I don't think anyone would want that. Unknown key/unrooted certificate is just "untrusted" content from security POV much like unsigned code.
I think to implement what you are suggesting we just need to change this in CertificateChecker.checkCertificates(SignedContentFactory): private IStatus checkCertificates(SignedContentFactory verifierFactory) { UIServices serviceUI = agent.getService(UIServices.class); ArrayList<Certificate> untrustedCertificates = new ArrayList<>(); Map<IArtifactDescriptor, Collection<PGPPublicKey>> untrustedPGPArtifacts = new HashMap<>(); Map<IArtifactDescriptor, File> unsigned = new HashMap<>(); ArrayList<Certificate[]> untrustedChain = new ArrayList<>(); Map<Certificate, Collection<File>> untrustedArtifacts = new HashMap<>(); IStatus status = Status.OK_STATUS; String policy = getUnsignedContentPolicy(); if (artifacts.isEmpty() || serviceUI == null || EngineActivator.UNSIGNED_ALLOW.equals(policy)) { return status; } I.e., if the policy is "allow" for unsigned content, we don't do any other checking (and do not call the UIServices with anything). The policy check at this subsequent line could also be removed: String[] details = EngineActivator.UNSIGNED_ALLOW.equals(policy) || unsigned.isEmpty() ? null I don't see any strong argument not to take this approach. Your arguments all seem reasonable and correct.
So we're all agreed that such a change is appropriate? I would then change the method to start like this: private IStatus checkCertificates(SignedContentFactory verifierFactory) { UIServices serviceUI = agent.getService(UIServices.class); IStatus status = Status.OK_STATUS; String policy = getUnsignedContentPolicy(); if (artifacts.isEmpty() || serviceUI == null || EngineActivator.UNSIGNED_ALLOW.equals(policy)) { return status; } No point in creating all those things that aren't used. And remove the guard later in the code...
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/190618
Gerrit change https://git.eclipse.org/r/c/equinox/rt.equinox.p2/+/190618 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=5f99bda74604fbdd5ffe3ea9bcfa410b0b4a0fe8
Verified with I20220209-1800 Thanks Ed!
