Allow GPG key fingerprints.

[naftulikay]

Patch to allow GPG key fingerprints as key values.

[naftulikay]

This patch allows 40-digit GPG fingerprints as key values in addition to 8 and 16-digit key IDs.

[naftulikay]

This patch needs support for checking if the key is already present, which it fails to do now.

[daenney]

This is a good idea, especially with regard to the recent short-id collisions, one of them involving the Puppet Labs key.

Do you have a plan on how to deal with the fingerprints in apt-key?

[naftulikay]

I'm currently looking into that. There's a way to do it from gpg, but I'm trying to find a way to ensure that it works properly from apt-key.

The key collision for the Puppet signing key is scary, noticed that last night.

[WolverineFan]

Does this have to wait on the check for already present? There are already 2 pull requests to address the 40 bit key problem (and I was about to submit a 3rd). Maybe we can get this in and work on the already present check separately? It does work, even if it's not perfect.

[WolverineFan]

Would this work: apt-key adv --list-keys --with-colons --fingerprint

Not the prettiest output, but gives the 40 character fingerprints.

[naftulikay]

apt-key adv --list-keys --with-colons --fingerprint | grep "fpr:::::::::$fingerprint:" >/dev/null

This works as expected.

[WolverineFan]

Also note that the pub lines in --with-colons output contain the 16 character keys. So between "apt-key list" and "apt-key adv --list-keys --with-colons --fingerprint" you can get all 3 versions of the keys for validation. Why there isn't a single command that gives all 3 versions is a mystery....

[daenney]

@WolverineFan apt-key has long since baffled me, the fact that you can't get it to show the long key ID is a complete mystery to me too.

Thanks for figuring out the commands though, I'll see if I can whip up something during the contrib summit at Puppetconf. Should allow a few people to weigh in too and we can get this fixed once and for all.

[WolverineFan]

Any chance this patch can get merged while you work on the better validation? Currently 40 character fingerprints just barf completely and this patch fixes that problem at least.

[daenney]

So me and @mhaskel would like to merge this because we're gearing up for a release but we can't do this without tests. It hasn't broken the current behaviour but we do need tests to prove that it works with the fingerprints too.

[underscorgan]

@rfkrocktk can you also update the documentation with the updated support for GPG key fingerprints?

[naftulikay]

Life is a bit hectic right now but I'll try to get this in within the next
week.

On Wed, Sep 24, 2014 at 3:00 PM, Morgan Haskel [email protected]
wrote:

@rfkrocktk https://github.com/rfkrocktk can you also update the
documentation with the updated support for GPG key fingerprints?

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

[daenney]

@rfkrocktk Did you get a chance to look at this?

[naftulikay]

I'm back at work next week, it'll have to wait until at least then, sorry
:-(
On Oct 15, 2014 7:18 AM, "Daniele Sluijters" [email protected]
wrote:

@rfkrocktk https://github.com/rfkrocktk Did you get a chance to look at
this?

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

[daenney]

No problem, but good to know. Thank you!

[daenney]

Ping?

[naftulikay]

I've resumed work on this and am looking into it.

I'm trying now to find a command I can use to list out all keys, their expiry, and their fingerprints so I can update lib/puppet/provider/apt_key.rb:28-55 to use this command instead of just apt-key list.

I'm specifically trying to find a command that outputs 8-digit key IDs, 16-digit key IDs, and 40-digit key fingerprints so that all checks will work properly.

After I figure this out (see relevant question on unix.stackexchange), I'll update the tests and hopefully we'll see this get merged.

The previous command apt-key adv --list-keys --with-colons --fingerprint works to get the key fingerprint alone, but not any of the IDs for the keys. The way that the current code works, it runs a single command apt-key list and iterates through the results. Ideally, we'll try to find a command to replace this which output delimited lines including 8 and 16-digit key IDs in addition to each key's 40-digit fingerprint.

[naftulikay]

Alright, so I've found a way to fix it, but I'm really not sure how some of the Puppet provider internals work.

How do I run the tests locally to make sure my changes work properly?

[underscorgan]

@rfkrocktk you can run the unit tests with bundle exec rake spec, or the acceptance tests with bundle exec rspec spec/acceptance (you'll need to have vagrant installed for that to work). There's some more info for the acceptance tests at https://github.com/puppetlabs/beaker/wiki/How-to-Write-a-Beaker-Test-for-a-Module

[naftulikay]

I have a design problem that I'd like to get some input on.

In the apt-key provider, it currently uses only 8-digit identifiers for key ids and if I were to replace them with the 40-digit fingerprints exclusively, this would introduce backwards-incompatible changes.

Here's what I've done so far in the apt-key provider. I've rewritten the following methods:

def self.instances
  cli_args = "adv --list-keys --with-colons --fingerprint"

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

  pub_line = nil
  fpr_line = nil

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

    next unless not (pub_line and fpr_line)

    line_hash = key_line_hash(pub_line, fpr_line)

    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],
      :fingerprint => line_hash[:key_fingerprint],
      :ensure  => :present,
      :expired => expired,
      :expiry  => line_hash[:key_expiry],
      :size    => line_hash[:key_size],
      :type    => line_hash[:key_type],
      :created => line_hash[:key_created]
    )

    # reset everything
    pub_line = nil
    fpr_line = nil
  end

  return key_array.compact!
end

# ...

def self.key_line_hash(pub_line, fpr_line)
  pub_split = pub_line.split(':')
  fpr_split = fpr_line.split(':')

  return_hash = {
    :key_id          => fpr_split.last[-8..-1], # last 8 characters is id
    :key_size        => pub_split[2],
    :key_type        => nil,
    :key_created     => pub_split[5],
    :key_expiry      => nil,
    :key_fingerprint => fpr_split.last,
  }

  # 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
  end

  # set key expiry
  if not pub_split[6].empty?
    return_hash[:key_expiry] = pub_split[6]
  end

  return return_hash
end

This provider makes it easy to parse the output of the command mentioned above to get everything from the key ids, expiry and created dates, key size and type, fingerprint, etc.

I'm new to writing Ruby Puppet code, so I'm not sure where the actual comparison is ever done to ensure that a given key is present. I can't seem to find a method I can rewrite to do the following:

def equals?(other)
  if self.id.length == 40
    # compare the fingerprint directly
    return self.fingerprint == other.fingerprint

Where is this done? I've done almost all of the work here, I just need to wrap things up to make sure that comparison is done properly. I'd like to support comparison between all three key types (short 8, long 16, fingerprint 40).

[daenney]

That's done for you. What this provider does is prefetch all the resources for you and generate one for every key that exists. Then it takes the namevar of the resource you've declared (the type declares which attribute is the namevar) and looks for if it has a resource that matches in the keys it prefetched.

This is done here: https://github.com/puppetlabs/puppetlabs-apt/blob/master/lib/puppet/type/apt_key.rb#L28

The munge part is important, whatever that returns is what we compare to our list of prefetched resources.

As you can see the id attribute is the namevar and is transformed depending on wether it's a hexadecimal representation of the key or not. That's probably where you need to tinker.

[naftulikay]

Actually, it seems that some of the logic is already done in the provider's prefetch method:

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
    end
  end
end

If I emend this to add another if statement to set the provider on resources[name] if its ID is 40 characters long and matches another one's fingerprint in apt_keys.

Is that right?

The problem I'm seeing is the mismatch between what the user supplies and requests that we ensure and the data we actually are providing. Since the user can supply an 8, 16, or 40 character ID, matching that to what we find in the back is problematic. Does the emendation I recommended above solve the problem?

[cyberious]

This will not be idempotent yet as the apt_key will always base the match on the short key. The aforementioned shortname was removed at 87f3f10 , we should either munge the name to be the shortname or reimplement the prefetch munging.

[naftulikay]

@cyberious I've pushed all of my modifications, please take a look. Feel free to complete the work on your own (if you know more about writing Ruby for Puppet than I do, which is easily possible), or at least tell me what changes I should make in order to complete the work on matching fingerprints properly.

[WolverineFan]

I think shortening the long key in 87f3f10 is bad. That's going to lead to collisions (maybe that's what you meant). It would be better to save all the variants and use the length to figure out which key to check. I tried a quick hack to do that and it doesn't work the way I expected. Maybe I'm not understanding how the prefetch method works.

[cyberious]

Prefetch has all of the keys ahead of time and can not figure out the ID length. A possible solution is to add a property that we would read in as well for the fingerprint and upon prefetch if the id == 40 than we would match based on fingerprint rather than id/name.

[WolverineFan]

This is what I was thinking, but my Ruby (and Puppet) are not great, so maybe this can't work?

We would store each option in the apt_keys hash and use the name length to figure out which key to match on.

   def self.prefetch(resources)
     apt_keys = instances
     resources.keys.each do |name|

     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

[cyberious]

that logic looks correct.

[cyberious]

However with that we need to add the properties for long short and fingerprint.

[WolverineFan]

I did that too:

   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_fingerprint => fingerprint,
      :key_long        => fingerprint[-16..-1], # last 16 characters of fingerprint
      :key_short       => fingerprint[-8..-1], # last 8 characters of fingerprint

And referenced them in the self.instances function. Maybe some other change I made is causing the failure. I'll do some more experimenting today if I can get the tests running locally

[naftulikay]

Thanks everyone for helping out on this. You're planning on adding tests to
cover this exact case, right? Getting the tests to pass is one thing, but
covering this new case is another, both are really important.
On Jan 8, 2015 9:46 AM, "WolverineFan" [email protected] wrote:

I did that too:

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_fingerprint => fingerprint,
  :key_long        => fingerprint[-16..-1], # last 16 characters of fingerprint
  :key_short       => fingerprint[-8..-1], # last 8 characters of fingerprint

And referenced them in the self.instances function. Maybe some other
change I made is causing the failure. I'll do some more experimenting today
if I can get the tests running locally

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

[WolverineFan]

Very true. I'm going to rebase all of this on current master and merge it all into a single commit to keep it pretty.

[naftulikay]

Thank you so much, it'll be nice to see this hit production :)

On Thu, Jan 8, 2015, 11:22 AM WolverineFan [email protected] wrote:

Very true. I'm going to rebase all of this on current master and merge it
all into a single commit to keep it pretty.

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

[WolverineFan]

Opened #404 with the new tests and bug fixes

[daenney]

I'm going to close this in favour of #404.