Allow full length GPG key fingerprints. #404

WolverineFan · 2015-01-09T03:43:49Z

This is a rebased version of rfkrocktk's patch #354 with added tests and several bug fixes found while running said tests. I think this should be good to go.

daenney · 2015-01-09T16:55:13Z

spec/defines/key_spec.rb

@@ -2,7 +2,8 @@

 describe 'apt::key', :type => :define do
  let(:facts) { { :lsbdistid => 'Debian' } }
-  GPG_KEY_ID = '4BD6EC30'
+  #GPG_KEY_ID = '4BD6EC30'


The comments serve no purpose, just remove them. We've got Git's history powers if we ever wanted to know what this line looked like in the past.

Yes, this is a major pet peeve. Commented code adds nothing unless its an example. Having Git means there's should be no such thing as "I'll comment this out in case I need it later."

I almost did, but it was late and I'd spent a couple hours rebasing/reformatting :) I'll remove them.

Also add support for ECC and ECDSA key_types

WolverineFan · 2015-01-10T05:53:44Z

Updated based on feedback and rebased on current master again. Can someone take a look at my #403 pull request too? The apt_has_updates fact doesn't work at all at the moment.

daenney · 2015-01-12T19:32:32Z

@WolverineFan Don't do that please, asking to push through another PR through this one unless they're related. We'll get to it, but we can't 24/7 our lives on this.

Allow full length GPG key fingerprints.

daenney · 2015-01-12T19:33:12Z

I've merged this so we don't sit on it any longer and the PR diverges again. It seems good, tests pass, lets see what happens next.

naftulikay · 2015-01-12T19:42:38Z

Great job everyone!

cyberious · 2015-01-13T23:39:09Z

lib/puppet/provider/apt_key/apt_key.rb

-        :type    => line_hash[:key_type] == 'R' ? :rsa : :dsa,
-        :created => line_hash[:key_created]
+        :name        => line_hash[:key_fingerprint],
+        :id          => line_hash[:key_fingerprint],


Is this really what we wanted, we are not reporting back the id as a fingerprint, previously this was the short.

True, but short is bad (not nearly unique enough). So I figured it had to change to long or fingerprint and chose fingerprint. It's a simple change back to short if it causes some breaking behavior (though I couldn't find anything that uses it).

This might be a candidate for a major version bump.
On Jan 14, 2015 7:31 PM, "WolverineFan" [email protected] wrote:

In lib/puppet/provider/apt_key/apt_key.rb
#404 (diff)
:

@@ -41,14 +49,17 @@ def self.instances
end

new(

:name => line_hash[:key_id],

:id => line_hash[:key_id],

:ensure => :present,

:expired => expired,

:expiry => line_hash[:key_expiry],

:size => line_hash[:key_size],

:type => line_hash[:key_type] == 'R' ? :rsa : :dsa,

:created => line_hash[:key_created]

:name => line_hash[:key_fingerprint],

:id => line_hash[:key_fingerprint],

True, but short is bad (not nearly unique enough). So I figured it had to
change to long or fingerprint and chose fingerprint. It's a simple change
back to short if it causes some breaking behavior (though I couldn't find
anything that uses it).

—
Reply to this email directly or view it on GitHub
https://github.com/puppetlabs/puppetlabs-apt/pull/404/files#r22988859.

Also, the build is broken.
On Jan 14, 2015 7:40 PM, "Naftuli Tzvi Kay" [email protected] wrote:

This might be a candidate for a major version bump.
On Jan 14, 2015 7:31 PM, "WolverineFan" [email protected] wrote:

In lib/puppet/provider/apt_key/apt_key.rb
#404 (diff)
:

@@ -41,14 +49,17 @@ def self.instances
end

new(

:name => line_hash[:key_id],

:id => line_hash[:key_id],

:ensure => :present,

:expired => expired,

:expiry => line_hash[:key_expiry],

:size => line_hash[:key_size],

:type => line_hash[:key_type] == 'R' ? :rsa : :dsa,

:created => line_hash[:key_created]

:name => line_hash[:key_fingerprint],

:id => line_hash[:key_fingerprint],

True, but short is bad (not nearly unique enough). So I figured it had to
change to long or fingerprint and chose fingerprint. It's a simple change
back to short if it causes some breaking behavior (though I couldn't find
anything that uses it).

—
Reply to this email directly or view it on GitHub
https://github.com/puppetlabs/puppetlabs-apt/pull/404/files#r22988859.

WolverineFan · 2015-01-15T04:41:32Z

I'm pretty the failures aren't because of this patch. My guess is the failure is in the test framework because it is consistently reporting getting nil for things (ironically the failures are in the code I fixed in my other patch, but my version doesn't work either). Also, running the all tests on my box against current master works fine.

cyberious · 2015-01-15T05:26:19Z

No the failures were directly part of how we were deleting the keys. Previously the failures were only b/c there were duplicate keys and we would not delete the expired key but just the first one apt-key found. Now it wasn't deleting them at all.

WolverineFan · 2015-01-15T05:33:03Z

That's odd. The tests have several checks for key deletion, so I'm very surprised if that broke as part of this patch. Plus the tests that are listed as failing are all apt_has_updates checks that refer to unexpected results.

WolverineFan · 2015-01-15T05:41:42Z

Just realized I can pretty much guarantee this patch didn't break the tests: Nothing was committed to master between my last rebase and when it was merged. My last rebase passed all tests. So the exact same commit SHA passed once and failed the second time. That points to a testing environment issue.

(By the way, I'm not implying there aren't problems with the patch, just that the test failures aren't because of it)

naftulikay · 2015-01-15T05:48:39Z

The builds were passing until it was merged. I'm not saying it was the
patch, but it's quite an interesting coincidence.

On Wed, Jan 14, 2015 at 9:41 PM, WolverineFan [email protected]
wrote:

Just realized I can pretty much guarantee this patch didn't break the
tests: Nothing was committed to master between my last rebase and when it
was merged. My last rebase passed all tests. So the exact same commit SHA
passed once and failed the second time. That points to a testing
environment issue.

(By the way, I'm not implying there aren't problems with the patch, just
that the test failures aren't because of it)

—
Reply to this email directly or view it on GitHub
#404 (comment)
.

cyberious · 2015-01-15T06:11:27Z

It isn't just the travis tests that I am referencing but also the acceptance tests we run internally. But nevertheless they are working now. Also given the community feedback I have created a PR #410 which changes the delete to use fingerprints as well.

naftulikay · 2015-01-15T08:21:04Z

Now that this is involving changes to other places in the codebase, I'm even more in favor of a major version bump.

cyberious · 2015-01-15T17:08:15Z

@rfkrocktk the changes that have occurred do not warrant a major version bump. I will look at what has occurred outside of this scope of these PR's

naftulikay · 2015-01-15T17:11:01Z

Aren't they backwards-breaking changes?
On Jan 15, 2015 9:08 AM, "Travis Fields" [email protected] wrote:

@rfkrocktk https://github.com/rfkrocktk the changes that have occurred
do not warrant a major version bump. I will look at what has occurred
outside of this scope of these PR's

—
Reply to this email directly or view it on GitHub
#404 (comment)
.

cyberious · 2015-01-15T18:10:37Z

How so? The API now takes 8, 16 or 40, we have added. I don't see it as a breaking change, we have exactly the same behavior just different mechanism of doing it. My only concern was that the puppet resource apt_key now does not report the namevar as short key.

naftulikay · 2015-01-15T18:39:48Z

My apologies, I thought that there were breaking changes, nevermind.
On Jan 15, 2015 10:10 AM, "Travis Fields" [email protected] wrote:

How so? The API now takes 8, 16 or 40, we have added. I don't see it as a
breaking change, we have exactly the same behavior just different mechanism
of doing it. My only concern was that the puppet resource apt_key now does
not report the namevar as short key.

—
Reply to this email directly or view it on GitHub
#404 (comment)
.

WolverineFan mentioned this pull request Jan 9, 2015

Allow GPG key fingerprints. #354

Closed

daenney reviewed Jan 9, 2015
View reviewed changes

Allow full length GPG key fingerprints.

445ad0b

Also add support for ECC and ECDSA key_types

WolverineFan force-pushed the support_40char_fingerprints branch from 41b04f3 to 445ad0b Compare January 10, 2015 05:49

daenney added a commit that referenced this pull request Jan 12, 2015

Merge pull request #404 from WolverineFan/support_40char_fingerprints

1a7d079

Allow full length GPG key fingerprints.

daenney merged commit 1a7d079 into puppetlabs:master Jan 12, 2015

cyberious reviewed Jan 13, 2015
View reviewed changes

cyberious mentioned this pull request Jan 14, 2015

MODULES-1661 Fix issue with apt_key destroy, also added mutliple deletes #408

Merged

WolverineFan deleted the support_40char_fingerprints branch January 15, 2015 04:01

LukasAud added the feature label Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow full length GPG key fingerprints. #404

Allow full length GPG key fingerprints. #404

WolverineFan commented Jan 9, 2015

daenney Jan 9, 2015

naftulikay Jan 9, 2015

WolverineFan Jan 10, 2015

WolverineFan commented Jan 10, 2015

daenney commented Jan 12, 2015

daenney commented Jan 12, 2015

naftulikay commented Jan 12, 2015

cyberious Jan 13, 2015

WolverineFan Jan 15, 2015

naftulikay Jan 15, 2015

naftulikay Jan 15, 2015

WolverineFan commented Jan 15, 2015

cyberious commented Jan 15, 2015

WolverineFan commented Jan 15, 2015

WolverineFan commented Jan 15, 2015

naftulikay commented Jan 15, 2015

cyberious commented Jan 15, 2015

naftulikay commented Jan 15, 2015

cyberious commented Jan 15, 2015

naftulikay commented Jan 15, 2015

cyberious commented Jan 15, 2015

naftulikay commented Jan 15, 2015

Allow full length GPG key fingerprints. #404

Allow full length GPG key fingerprints. #404

Conversation

WolverineFan commented Jan 9, 2015

daenney Jan 9, 2015

Choose a reason for hiding this comment

naftulikay Jan 9, 2015

Choose a reason for hiding this comment

WolverineFan Jan 10, 2015

Choose a reason for hiding this comment

WolverineFan commented Jan 10, 2015

daenney commented Jan 12, 2015

daenney commented Jan 12, 2015

naftulikay commented Jan 12, 2015

cyberious Jan 13, 2015

Choose a reason for hiding this comment

WolverineFan Jan 15, 2015

Choose a reason for hiding this comment

naftulikay Jan 15, 2015

Choose a reason for hiding this comment

naftulikay Jan 15, 2015

Choose a reason for hiding this comment

WolverineFan commented Jan 15, 2015

cyberious commented Jan 15, 2015

WolverineFan commented Jan 15, 2015

WolverineFan commented Jan 15, 2015

naftulikay commented Jan 15, 2015

cyberious commented Jan 15, 2015

naftulikay commented Jan 15, 2015

cyberious commented Jan 15, 2015

naftulikay commented Jan 15, 2015

cyberious commented Jan 15, 2015

naftulikay commented Jan 15, 2015