Puppet

I’ve attached a patch that is my workaround for the problem. It works if the installation was previously using the default “puppet” server name and it’s CNAME friendly.

zothar: Sorry, this will only work in a subset of scenarious :–( We really do need the subjectAltName stuff added. I’ll see what I can do. Derek.

Here are a trio of patches ({ca,defaults,sslcertificates}.patch) which add a new configuration parameter, “certdnsnames”. This defaults to *

It is a colon-separated list of the DNS names to be recorded in the subjectAltName of the server certificate generated for the Puppet Master server. Once your latest ‘n’ greatest Ruby bothers to check this (:–) the client will only accept the server’s cert when the DNS name it connects to matches one of the certdnsnames. Wildcards are permitted as per OpenSSL’s usual fun and games.

I haven’t added IP: or any of the other forms serverAltName supports.

Cheers, Derek

This has hit RHEL5.1 – ruby-1.8.5-5el5_1.1 has the stricter checking as per bz 313691 but RHEL5.0 – ruby-1.8.5-5el5 does not.

Just as an FYI, I think the best way to patch this problem is to check for, and then turn off the new, default behavior of Ruby’s SSL checking.

For example, in ruby 1.8.5, according to [http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=rev&revision=13502]

the new checks may be overridden with:

Net::HTTP#enable_post_connection_check=(false)

We’re already overriding some methods in Net::HTTP in client.rb in 0.23.2.

More information about the security change is available at: http://www.isecpartners.com/advisories/2007-006-rubyssl.txt

Also, DerekW mentioned in IRC that this issue really only affects the puppetmaster server certificate, whose certificate must match the requrest string of the puppetd client connection.

And just as another comment, requests to the mailing list or IRC should probably be pointed at RubySSL-2007-006

I’ve published a small, untested patch. I’ll reply back once I’ve had a chance to actually test this patch, and write an additional test-case for the situation.

The branch is named ‘pending/fix-896’ and is located at: [http://northstarlabs.net/git/puppet]

New defaults.rb patch attached. This is against a git checkout of 20/11/07, i.e. it’s cumulative and doesn’t replace what’s already submitted/checked in.

You now get ,.,..,...,....,.....* as your default certdnsnames, good enough to match most people domains.

I’ve applied Derek’s patch but cannot reach Jeff’s repo, apparently.

Replying to [comment:13 luke]:

I’ve applied Derek’s patch but cannot reach Jeff’s repo, apparently.

My patch to completely disable the new behavior is now published as commit:36c947ecae1b0082cf535dc691891b6cd7bf2c51 in the branch refs/heads/pending/fix-896r2

Hopefully that works.

I’ve also added a conditional to check if Net::HTTP#enable_post_connection_check needs to be defined or not. This should provide maximum compatibility with future and past versions of Ruby.

For those being bitten by this bug currently, I’ve backported this patch to 0.23.2.

To apply the patch:

# Get the current code
git clone git://reductivelabs.com/puppet/ puppet.git
cd puppet.git

# Stage version 0.23.2
git checkout 0.23.2

# Create a new local branch for the backported patch.
git checkout -b 0.23.2p1

# Fetch, merge, and commit the patch fix.
git remote add mccune http://northstarlabs.net/git/puppet
git pull mccune backports/0.23.2/fix-896

To see the changes made:

git diff HEAD^

Hope this helps, as this seems to be impacting a lot of people.

This patched client will need to be installed on any machines with a “fixed” (recent) ruby.

I’ve tested all these patches and none worked with dlutters rpm and the latest rhel4 ruby libs

the ruby lib …

        if @socket.socket.context.verify_mode != [[OpenSSL]]::SSL::VERIFY_NONE
          @socket.socket.post_connection_check(@address)
        end

/network/xmlrpc/client.rb

@http.verify_mode = [[OpenSSL]]::SSL::VERIFY_NONE

Replying to [comment:17 semantico]:

I’ve tested all these patches and none worked with dlutters rpm and the latest rhel4 ruby libs

the ruby lib …

    if @socket.socket.context.verify_mode != [[OpenSSL]]::SSL::VERIFY_NONE
      @socket.socket.post_connection_check(@address)
    end

/network/xmlrpc/client.rb

@http.verify_mode = [[OpenSSL]]::SSL::VERIFY_NONE

Am I right in understanding that this basically disables OpenSSL’s verification, thus reverting puppet to the behavior Ruby was having before the security patch?

Replying to [comment:18 anarcat]:

Replying to [comment:17 semantico]:

the ruby lib …

    if @socket.socket.context.verify_mode != [[OpenSSL]]::SSL::VERIFY_NONE
      @socket.socket.post_connection_check(@address)
    end

/network/xmlrpc/client.rb

@http.verify_mode = [[OpenSSL]]::SSL::VERIFY_NONE

Am I right in understanding that this basically disables OpenSSL’s verification, thus reverting puppet to the behavior Ruby was having before the security patch?

Yes I believe so, but the reason that semantico is doing this is because semantico said:

I’ve tested all these patches and none worked with dlutters rpm and the latest rhel4 ruby libs

Which to me means that the patches didn’t work for semantico, so a crude work-around is to disable things altogether.

Argh, I’m almost certain my patch (just disabling the post_connection_check) is correct, but I’ll triple test it right now.

I agree with micah, it seems semantico is just disabling SSL verification altogether, which will obviously “work”.

semantico, could you please verify that you’ve tested my patch, and only my patch against 0.23.2 ? There’s a back-ported version available, and I’ll post a patch file if you don’t have access to git.

Perhaps this should be two tickets to save confusion. I believe my patches may be mutually exclusive with whaymand_home and zothar’s patches.

-mccune

OK, so I just extensively tested this problem and my solution, and I believe I’ve wrapped my head around the issue completely and worked out a solution that works for everyone.

I believe we should disregard all patches except mine. (Sorry everyone else).

Here’s my motivation:

First, the ruby maintainers did not actually change the behavior of ruby. They merely added the post_connection_check, which issues a warning if the certificate CN does not match the request string passed to Net::HTTP.

Second, it is the vendors who are taking the extra security step and setting

@enable_post_connection_check = true

in net/http.rb. Ruby maintainers default to false, which is the behavior puppet users have expected until now.

Third, all other submitted patches appear to drastically change how certificates are generated, which is not necessary if

@enable_post_connection_check = false

Finally, the patch I’ve submitted simply “undoes” what vendors are doing. That is, the patch rolls back to the behavior of allowing the SSL connection to continue, with a warning, if the server certificate’s subject field contains a CN string other than what is specified to the —server option of puppetd.

In addition, not all vendors are setting

@enable_post_connection_check = true

, which means any change in behavior to the way certificates are generated is not necessary when used with a ruby package not setting this condition true.

If anyone else would like to verify my efforts, the trick is to start with http://ftp.ruby-lang.org/pub/ruby/1.8/ruby-1.8.6-p111.tar.gz or http://ftp.ruby-lang.org/pub/ruby/1.8/ruby-1.8.5-p114.tar.gz and make sure to set

@enable_post_connection_check = true

instead of false in lib/ruby/1.8/net/http.rb, in order to mimic the behavior of the security conscience vendors.

Phew. So, in summary:

for HEAD see: http://northstarlabs.net/cgi-bin/gitweb.cgi?p=puppet;a=commitdiff;h=36c947ecae1b0082cf535dc691891b6cd7bf2c51

for 0.23.2 see: http://northstarlabs.net/cgi-bin/gitweb.cgi?p=puppet;a=commitdiff;h=c0e2ecb71d238740a4d831d83722c70ad498219a

Cheers, -Jeff

Jeff, your patch works perfectly (tested the backported 0.23 version from your website) with current RH Ruby from 5.0 and 5.1, and also 4.4 (which hey, doesn’t even need it). The only downer is that is requires a new Puppet client package. With subjectAltName you only require a new server package (or a one-off hack to mint some certs). Less intrusive. Either way, we (Barcap) have to run with patches for the short term anyway so I have no personal or professional vested interest in my patch above Jeff’s. It’s more what the community wants or what the overall architectural plan is that matters.

What really does matters is that this is fixed, one way or another in 0.24 :–)

Regards, Derek

Replying to [comment:22 whaymand]:

Jeff, your patch works perfectly (tested the backported 0.23 version from your website) with current RH Ruby from 5.0 and 5.1, and also 4.4 (which hey, doesn’t even need it).

Thanks for verifying this.

The only downer is that is requires a new Puppet client package.

Correct.

With subjectAltName you only require a new server package (or a one-off hack to mint some certs). Less intrusive.

Ah, to be a bit more clear, if you do the one-off hack to mint a new SSL certificate for the server, no patching is necessary whatsoever. This is actually the solution route I’ve taken at my site.

I’m perfectly happy to document a fairly straightforward way to mint a new certificate manually, using the Makefile and openssl.cnf from http://sial.org/ similar to what I did with MultipleCertificateAuthorities. If anyone is interested in that HOWTO, just let me know.

What really does matters is that this is fixed, one way or another in 0.24 :–)

Agreed. It’s looking like it will be, so I think we’re in good shape.

Replying to [comment:23 mccune]:

Ah, to be a bit more clear, if you do the one-off hack to mint a new SSL certificate for the server, no patching is necessary whatsoever. This is actually the solution route I’ve taken at my site.

I’m perfectly happy to document a fairly straightforward way to mint a new certificate manually, using the Makefile and openssl.cnf from http://sial.org/ similar to what I did with MultipleCertificateAuthorities. If anyone is interested in that HOWTO, just let me know.

I think this would be very important for people that are about to do their Ruby upgrade on puppet installations. As far as I know, the place for that is RubySSL-2007-006 right now, but from what micah said on IRC, the instructions there are not working.

I’m also worried about some patches here that disable SSL verifications, does your solution disable the verification too?

Shouldn’t the proper solution be that the puppetmaster generate the right certificate in the first place?

Disclaimer: I’m having a hard time figuring out exactly what the problem is around this issue, so take my comments with a proverbial grain of salt.

Replying to [comment:25 luke]:

Does anyone think this should be an option for the more anal security folks?

Possibly, but I’d leave it up to the anal security folks to implement it if they’re worried about it. For me, hostname validation is only important when signatures from a CA are trivial for anyone to get; in Puppet’s security model, it isn’t important that the CN matches the hostname because the very presence of a signature from the CA says “yes, this is host is OK”.

Cool; I’ll leave it at that, then.

I think the last patch (disabling hostname checking altogether) is a horrible idea – sorry for being so blunt. It means that (a) puppet changes the behavior of ruby’s ssl connections in a subtle way that’s almost impossible to find for the average user and (b) makes broken behavior that is serious enough to have a CVE filed for it the default with no way to turn it off.

I also don’t understand why that change is needed; there are several perfectly good ways to make puppet work with the fixed ruby SSL implementation, at least one of which only requires server-side changes.

Replying to [comment:28 lutter]:

I think the last patch (disabling hostname checking altogether) is a horrible idea – sorry for being so blunt.

No need to apologize, at least to me. I like blunt.

After considering it more, I think you’re completely right. Were I not the one who disabled the SSL post connection check, I would be surprised to find out that it wasn’t happening if I a new puppet user.

I’ll augment the patch to make a configurable option, with the default behavior following that of Ruby’s default behavior.

I hope to have this done by the end of the weekend, if not this afternoon.

I’ve patched this to be configurable. Please see

refs/heads/pending/fix-896r3

of http://northstarlabs.net/git/puppet

As a reference, here’s the commitdiff:

http://northstarlabs.net/cgi-bin/gitweb.cgi?p=puppet;a=commitdiff;h=f94d6d3394dd0fa9ecf06b727cb7234fede7c960

Commit is commit:f94d6d3394dd0fa9ecf06b727cb7234fede7c960

A second commit, [24cacdb] Fixes the test case for enable_post_connection_check to match the configuration.

This fix causes failures in spec/unit/network/xmlrpc/client.rb; I’ve added new tests to that file, since it’s easier for me to think in rspec mode now.

Don’t forget us EL4-ers!

Jeff’s patch won’t work, since redhat didn’t backport the ability to turn off the SSL checks.

I attempted to follow the suggestion in RubySSL-2007-006 and re-created the certificate with server = puppet in my [puppetmasterd] config:

-----END CERTIFICATE-----
subject=/CN=puppet
issuer=/CN=netserv.math.ohiou.edu

But now, I get the following:

[hyclak@euclid puppet]$ sudo /usr/sbin/puppetd --test --server=puppet
notice: Ignoring cache
err: Could not retrieve configuration: Certificates were not trusted: data too large for modulus
warning: Not using cache on failed configuration

This has been seen before in #223, but I only have one puppetmaster running, and if I kill it, it really does die.

Replying to [comment:33 hyclak]:

Don’t forget us EL4-ers!

Jeff’s patch won’t work, since redhat didn’t backport the ability to turn off the SSL checks.

I attempted to follow the suggestion in RubySSL-2007-006 and re-created the certificate with server = puppet in my [puppetmasterd] config:

——-END CERTIFICATE——–
subject=/CN=puppet
issuer=/CN=netserv.math.ohiou.edu

But now, I get the following:

[hyclak@euclid puppet]$ sudo /usr/sbin/puppetd —test —server=puppet
notice: Ignoring cache
err: Could not retrieve configuration: Certificates were not trusted: data too large for modulus
warning: Not using cache on failed configuration

This has been seen before in #223, but I only have one puppetmaster running, and if I kill it, it really does die.

You can probably disregard this problem – I removed all traces of puppet from all clients and servers, reinstalled, and now things seem to be working fine.

OK, I’ve updated the test cases. The problem is that I moved @http.enable_post_connection_check=true out of the cert_setup method and into the http_instance method. This broke the original test case.

I’ve removed the test, and agumented the spec tests, but I have no idea if I did this correctly. Could you glance at the latest commits to

mccune/pending/fix-896r3

and see if they’re reasonable?

The commitdiff is at: http://northstarlabs.net/cgi-bin/gitweb.cgi?p=puppet;a=commitdiff;h=a012849e9ca496ccf72cbaf307f220f3891b802e

JFYI: the current implementation of certdnsnames has a serious, but easy fixable problem. I posted it as a new bug at #942, so it isn’t lost here.

Applied whaymond_home’s patch for #942 in commit:5886d37af0429728db42faf7e950d971145a643b.

Also merged and pushed mccune’s fix.

This fix works, but I’d still like to see a patch that added ‘puppet’ as a default alias for the server and didn’t use the ‘.*.’ aliases. At this point, Puppet makes all certs less secure — we’re essentially obviating the post-connection check because we’ve got these aliases set up.

I’m going to leave this ticket open until that happens, I think.