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

Allow full length GPG key fingerprints. #404

Merged
merged 1 commit into from
Jan 12, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 66 additions & 64 deletions lib/puppet/provider/apt_key/apt_key.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,42 +13,53 @@

Puppet::Type.type(:apt_key).provide(:apt_key) do

KEY_LINE = {
:date => '[0-9]{4}-[0-9]{2}-[0-9]{2}',
:key_type => '(R|D)',
:key_size => '\d{4}',
:key_id => '[0-9a-fA-F]+',
:expires => 'expire(d|s)',
}

confine :osfamily => :debian
defaultfor :osfamily => :debian
commands :apt_key => 'apt-key'

def self.instances
cli_args = ['adv','--list-keys', '--with-colons', '--fingerprint']

if RUBY_VERSION > '1.8.7'
key_output = apt_key('list').encode('UTF-8', 'binary', :invalid => :replace, :undef => :replace, :replace => '')
key_output = apt_key(cli_args).encode('UTF-8', 'binary', :invalid => :replace, :undef => :replace, :replace => '')
else
key_output = apt_key('list')
key_output = apt_key(cli_args)
end

pub_line, fpr_line = nil

key_array = key_output.split("\n").collect do |line|
line_hash = key_line_hash(line)
next unless line_hash
if line.start_with?('pub')
pub_line = line
elsif line.start_with?('fpr')
fpr_line = line
end

next unless (pub_line and fpr_line)

line_hash = key_line_hash(pub_line, fpr_line)

# reset everything
pub_line, fpr_line = nil

expired = false

if line_hash[:key_expiry]
expired = Date.today > Date.parse(line_hash[:key_expiry])
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],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

:fingerprint => line_hash[:key_fingerprint],
:short => line_hash[:key_short],
:long => line_hash[:key_long],
:ensure => :present,
:expired => expired,
:expiry => line_hash[:key_expiry],
:size => line_hash[:key_size],
:type => line_hash[:key_type],
:created => line_hash[:key_created]
)
end
key_array.compact!
Expand All @@ -57,59 +68,50 @@ def self.instances
def self.prefetch(resources)
apt_keys = instances
resources.keys.each do |name|
if name.length == 16
shortname=name[8..-1]
else
shortname=name
end
if provider = apt_keys.find{ |key| key.name == shortname }
resources[name].provider = provider
if name.length == 40
if provider = apt_keys.find{ |key| key.fingerprint == name }
resources[name].provider = provider
end
elsif name.length == 16
if provider = apt_keys.find{ |key| key.long == name }
resources[name].provider = provider
end
elsif name.length == 8
if provider = apt_keys.find{ |key| key.short == name }
resources[name].provider = provider
end
end
end
end

def self.key_line_hash(line)
line_array = line.match(key_line_regexp).to_a
return nil if line_array.length < 5
def self.key_line_hash(pub_line, fpr_line)
pub_split = pub_line.split(':')
fpr_split = fpr_line.split(':')

fingerprint = fpr_split.last
return_hash = {
:key_id => line_array[3],
:key_size => line_array[1],
:key_type => line_array[2],
:key_created => line_array[4],
:key_expiry => nil,
:key_fingerprint => fingerprint,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we keep the :key_id?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, shouldn't we keep the type, size, created, and expiry as well? More information would be better than less.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. Especially since those fields are still defined on the type. We're losing information here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove key_id as it is only the hash and holds no meaning seeing as we are matching on short / long or fingerprint. ID is really a non specific value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyberious is correct. key_id was was renamed key_short. The other values are below here, nothing was removed.

:key_long => fingerprint[-16..-1], # last 16 characters of fingerprint
:key_short => fingerprint[-8..-1], # last 8 characters of fingerprint
:key_size => pub_split[2],
:key_type => nil,
:key_created => pub_split[5],
:key_expiry => pub_split[6].empty? ? nil : pub_split[6],
}

return_hash[:key_expiry] = line_array[7] if line_array.length == 8
return return_hash
end
# set key type based on types defined in /usr/share/doc/gnupg/DETAILS.gz
case pub_split[3]
when "1"
return_hash[:key_type] = :rsa
when "17"
return_hash[:key_type] = :dsa
when "18"
return_hash[:key_type] = :ecc
when "19"
return_hash[:key_type] = :ecdsa
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add:

  • 18: ECC
  • 19: ECDSA

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have these, but IIRC, they're really rare. Elliptic Curve algorithms should be more secure with smaller key sizes, but you really have to go out of your way to use them when creating the keys.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but they're up and coming, I've already seen these keys in the wild. It's not that much work to add two case statements.


def self.key_line_regexp
# This regexp is trying to match the following output
# pub 4096R/4BD6EC30 2010-07-10 [expires: 2016-07-08]
# pub 1024D/CD2EFD2A 2009-12-15
regexp = /\A
pub # match only the public key, not signatures
\s+ # bunch of spaces after that
(#{KEY_LINE[:key_size]}) # size of the key, usually a multiple of 1024
#{KEY_LINE[:key_type]} # type of the key, usually R or D
\/ # separator between key_type and key_id
(#{KEY_LINE[:key_id]}) # hex id of the key
\s+ # bunch of spaces after that
(#{KEY_LINE[:date]}) # date the key was added to the keyring
# following an optional block which indicates if the key has an expiration
# date and if it has expired yet
(
\s+ # again with thes paces
\[ # we open with a square bracket
#{KEY_LINE[:expires]} # expires or expired
\: # a colon
\s+ # more spaces
(#{KEY_LINE[:date]}) # date indicating key expiry
\] # we close with a square bracket
)? # end of the optional block
\Z/x
regexp
return return_hash
end

def source_to_file(value)
Expand Down
30 changes: 27 additions & 3 deletions lib/puppet/type/apt_key.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
newparam(:id, :namevar => true) do
desc 'The ID of the key you want to manage.'
# GPG key ID's should be either 32-bit (short) or 64-bit (long) key ID's
# and may start with the optional 0x
newvalues(/\A(0x)?[0-9a-fA-F]{8}\Z/, /\A(0x)?[0-9a-fA-F]{16}\Z/)
# and may start with the optional 0x, or they can be 40-digit key fingerprints
newvalues(/\A(0x)?[0-9a-fA-F]{8}\Z/, /\A(0x)?[0-9a-fA-F]{16}\Z/, /\A(0x)?[0-9a-fA-F]{40}\Z/)
munge do |value|
if value.start_with?('0x')
id = value.partition('0x').last.upcase
Expand Down Expand Up @@ -66,6 +66,30 @@
desc 'Additional options to pass to apt-key\'s --keyserver-options.'
end

newproperty(:fingerprint) do
desc <<-EOS
The 40-digit hexadecimal fingerprint of the specified GPG key.

This property is read-only.
EOS
end

newproperty(:long) do
desc <<-EOS
The 16-digit hexadecimal id of the specified GPG key.

This property is read-only.
EOS
end

newproperty(:short) do
desc <<-EOS
The 8-digit hexadecimal id of the specified GPG key.

This property is read-only.
EOS
end

newproperty(:expired) do
desc <<-EOS
Indicates if the key has expired.
Expand All @@ -92,7 +116,7 @@

newproperty(:type) do
desc <<-EOS
The key type, either RSA or DSA.
The key type, one of: rsa, dsa, ecc, ecdsa

This property is read-only.
EOS
Expand Down
10 changes: 6 additions & 4 deletions manifests/key.pp
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@
# [*key*]
# _default_: +$title+, the title/name of the resource
#
# Is a GPG key ID. This key ID is validated with a regex enforcing it
# to only contain valid hexadecimal characters, be precisely 8 or 16
# characters long and optionally prefixed with 0x.
# Is a GPG key ID or full key fingerprint. This value is validated with
# a regex enforcing it to only contain valid hexadecimal characters, be
# precisely 8 or 16 hexadecimal characters long and optionally prefixed
# with 0x for key IDs, or 40 hexadecimal characters long for key
# fingerprints.
#
# [*ensure*]
# _default_: +present+
Expand Down Expand Up @@ -57,7 +59,7 @@
$key_options = undef,
) {

validate_re($key, ['\A(0x)?[0-9a-fA-F]{8}\Z', '\A(0x)?[0-9a-fA-F]{16}\Z'])
validate_re($key, ['\A(0x)?[0-9a-fA-F]{8}\Z', '\A(0x)?[0-9a-fA-F]{16}\Z', '\A(0x)?[0-9a-fA-F]{40}\Z'])
validate_re($ensure, ['\Aabsent|present\Z',])

if $key_content {
Expand Down
Loading