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

Add parameter 'strictNoSignature' to make missing signatures explicit in keys map #44

Merged
merged 12 commits into from Nov 15, 2019

Conversation

@cobratbq
Copy link
Contributor

cobratbq commented Nov 14, 2019

Added maven configuration parameter strictNoSignature to express missing signatures for individual artifacts akin to how public keys are specified, in the keys map.

The following use cases are covered:

  • This provides support for issue #16:
  • Ability to detect new artifacts that are missing signatures in set-up where there exist already some artifacts that are missing a signature. (Use case where failNoSignature is too strict.)
  • Ability to make explicit for which artifacts we do not expect signatures.

Features:

  • quiet parameter is respected.
  • logs correct verification if artifact is missing signature consistent with keys map.
cobratbq added 6 commits Nov 14, 2019
…the keys map.

In order to detect and support the use cases "some version of artifact
comes without signature" and "strict preselection of artifacts having
no signature", the property 'strictNoSignature' will refer to the keys
map to verify whether or not having no signature is acceptable for
each unsigned artifact.

This mode is less strict than 'failNoSignature' as it will be able to
accomodate artifacts without signature. At the same time it provides
explicit control over which artifacts are acceptable without signature.
@cobratbq

This comment has been minimized.

Copy link
Contributor Author

cobratbq commented Nov 14, 2019

I haven't discussed the approach prior to implementation simply because the implementation turned out to be trivial. Let me know any feedback you might have or in case you want to take a different approach.

- Undid unnecessary change. Remnant of previous experiment.
- Updated documentation to better explain how the new configuration
parameter strictNoSignature works and what it expects from the keys map
content.
@slawekjaranowski

This comment has been minimized.

Copy link
Contributor

slawekjaranowski commented Nov 14, 2019

Thank you for your collaboration.

Please also look at sonar report result.

@@ -356,7 +376,7 @@ private void verifyArtifacts(Set<Artifact> artifacts)
for (Artifact artifact : artifacts) {
final Artifact ascArtifact = resolveAscArtifact(artifact);

if (ascArtifact != null) {
if (ascArtifact != null || strictNoSignature) {

This comment has been minimized.

Copy link
@slawekjaranowski

slawekjaranowski Nov 14, 2019

Contributor

move this logic to resolveAscArtifact method - so we don't need null to map

@@ -481,6 +501,9 @@ private void verifyArtifactSignatures(Map<Artifact, Artifact> artifactToAsc)

private boolean verifyPGPSignature(Artifact artifact, Artifact ascArtifact)
throws MojoFailureException {
if (ascArtifact == null) {

This comment has been minimized.

Copy link
@slawekjaranowski

slawekjaranowski Nov 14, 2019

Contributor

it will be not necessary if we don't put nulls

@@ -391,7 +411,7 @@ private Artifact resolveAscArtifact(Artifact artifact) throws MojoExecutionExcep
if (failNoSignature) {
getLog().error("No signature for " + artifact.getId());
throw new MojoExecutionException("No signature for " + artifact.getId());
} else {
} else if (!strictNoSignature) {

This comment has been minimized.

Copy link
@slawekjaranowski

slawekjaranowski Nov 14, 2019

Contributor

call verifySignatureUnavailable in this place and throw exception - so only this place will be changed

This comment has been minimized.

Copy link
@cobratbq

cobratbq Nov 15, 2019

Author Contributor

IIUC this will result in build failing at the first unlisted, unsigned artifact. So you lose the ability to list all unsigned artifacts, so the user can solve them in one go. Your thoughts?

@slawekjaranowski

This comment has been minimized.

Copy link
Contributor

slawekjaranowski commented Nov 14, 2019

some IT test will be good for code coverage

The weak signatures-check was mixed up with the PGP signature
verification result handling. This is not necessary, given that it is an
independent operation, which relies on very little information other
than the signature information. Hence moved out to improve readability.
@cobratbq

This comment has been minimized.

Copy link
Contributor Author

cobratbq commented Nov 14, 2019

@slawekjaranowski I have moved the weak signature-check up out of the PGP signature content check. This makes the logic a lot more readable and reduces nesting. Could you have a look if you agree, given that this reorders the checks and thus messaging order.

@cobratbq

This comment has been minimized.

Copy link
Contributor Author

cobratbq commented Nov 14, 2019

I will follow up on further comments later.

@slawekjaranowski slawekjaranowski added this to the v1.5.0 milestone Nov 14, 2019
@slawekjaranowski slawekjaranowski merged commit d3bd12f into s4u:master Nov 15, 2019
0 of 2 checks passed
0 of 2 checks passed
SonarCloud Code Analysis Quality Gate failed
Details
Travis CI - Pull Request Build Failed
Details
@slawekjaranowski

This comment has been minimized.

Copy link
Contributor

slawekjaranowski commented Nov 15, 2019

once again thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.