939543 - getcert.cgi should allow some host generate arbitrary certificates

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: Puppet, task)

Description

[Rail Aliiev [:rail]]

To reduce load on getcert.cgi (which sometimes reuses to generate new certs du to the ssl lock timeouts) and eliminate (almost) possible merge conflicts when we revoke certificates for spot instances it would be great to allow *some* IPs to generate certs for other hosts and cache them for N days.

Comment 1

[Rail Aliiev [:rail]]

Attachment: getcert.cgi

Maybe something like this? I haven't tested this at all.

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8334810 [details] [diff] [review]
getcert.cgi

Looks good.. I'd rather not skip DNS, though - can the manager provide IPs instead?

Comment 3

[Rail Aliiev [:rail]]

Attachment: getcert.cgi

--noop worked fine:

Notice: /Stage[main]/Puppetmaster::Deploy/File[/var/lib/puppetmaster/deploy/cgi-bin/getcert.cgi]/content: 
--- /var/lib/puppetmaster/deploy/cgi-bin/getcert.cgi    2013-11-18 12:20:46.403406284 -0800
+++ /tmp/puppet-file20131125-24338-fl9g-0       2013-11-25 09:11:25.131406777 -0800
[skipped]
+# IP addresses of machines allowed to generate arbitrary certificates
+my @manager_ips = (
+    qw/10.26.48.43/,
+
+);
+
@@ -52,6 +56,11 @@
+my $fqdn_regexps = [
+    qr/mozilla\.com$/m,
+
+];
+

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8337821 [details] [diff] [review]
getcert.cgi

Review of attachment 8337821 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/puppetmaster/templates/getcert.cgi.erb
@@ +72,5 @@
> +if ($ENV{'QUERY_STRING'}){
> +    # Explicit FQDN certificate request
> +    die "$ip is not allowed to generate certificates for other hosts" unless grep $_ eq $ip, @manager_ips;
> +    $host = $ENV{'QUERY_STRING'};
> +    die "Hostname must use letters, digits, dots or dashes" unless $host =~ /^[a-z0-9.-]+$/i;

Hm, this still gets the hostname from the query, rather than from DNS.  The requirement that an attacker spoof DNS adds a little bit more difficulty, and also limits the inputs to getcert.sh to avoid any string-quoting or other vulnerabilities that may exist there.  Is it problematic to use IPs here?

Comment 5

[Rail Aliiev [:rail]]

Attachment: getcert.cgi

* added checks for requested hostnames
* moved octet checks
* simplified IP/FQDN checks

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8337918 [details] [diff] [review]
getcert.cgi

Review of attachment 8337918 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/puppetmaster/templates/getcert.cgi.erb
@@ +55,5 @@
> +    die "$manager_ip is not allowed to generate certificates for other hosts" unless grep $_ eq $manager_ip, @$manager_ips;
> +    die "Hostname must use letters, digits, dots or dashes" unless $host =~ /^[a-z0-9.-]+$/i;
> +    $ip = inet_aton($host) or die "Cannot back resolve $host: $!";
> +    $ip = inet_ntoa($ip);
> +    my $real_host = gethostbyaddr(inet_aton($ip), AF_INET) or die "cannot resolve $ip: $!";

This is odd -- the regexp allows lots of characters, but you then validate with `inet_aton` and back, and then forward again.  Also, the error message for a failure in `inet_aton` is incorrect - that function doesn't do any kind of resolution.

I think the better approach would be:

 die "$manager_ip is not allowed to generate certificates for other hosts" unless grep $_ eq $manager_ip, @$manager_ips;	
 $ip = $ENV{'QUERY_STRING};
 die "Other host must be specified by IP" unless $ip =~ /^\d+\.\d+\.\d+\.\d+$/i;
 inet_aton($host) or die "Invalid IP: $!";

and then move the "# convert to a hostname" bit out of the conditional.

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

A few more comments from my conversation with :tinfoil, leading up to a secreview for this change:

- The security of the manager host will be important here, as it has a whole lot of power.  What host is it, and what kind of security restrictions are in place there?

- It would be ideal to require some extra authentication from the manager host, beyond the standard deploy password.  Ideally, this would be a client SSL certificate, which the manager host should already have from puppet.  The key would be getting Apache to validate that.

Comment 8

[Rail Aliiev [:rail]]

(In reply to Dustin J. Mitchell [:dustin] (I read my bugmail; don't needinfo me) from comment #7)
> A few more comments from my conversation with :tinfoil, leading up to a
> secreview for this change:
> 
> - The security of the manager host will be important here, as it has a whole
> lot of power.  What host is it, and what kind of security restrictions are
> in place there?

ATM it's cruncher, but I'm planning to setup another VM(s) dedicated to AWS management only, with similar security we have on buildbot masters.


> - It would be ideal to require some extra authentication from the manager
> host, beyond the standard deploy password.  Ideally, this would be a client
> SSL certificate, which the manager host should already have from puppet. 
> The key would be getting Apache to validate that.

Does this mean that getcert.cgi needs to check if the client is authenticated in case if we pass QUERY_STRING?

Comment 9

[Dustin J. Mitchell [:dustin] (he/him)]

A dedicated host sounds like a good plan.

And to your second question, yes.  I'm not sure how best to accomplish that, though - it will involve some Apache config.

Comment 10

[Rail Aliiev [:rail]]

Attachment: getcert.cgi

* use IP instead of FQDN for managed requests
* use inet_aton to verify IPs

Comment 11

[Rail Aliiev [:rail]]

Comment on attachment 8338579 [details] [diff] [review]
getcert.cgi

https://hg.mozilla.org/build/puppet/rev/41ffa94c897b

Comment 12

[Rail Aliiev [:rail]]

Comment on attachment 8338579 [details] [diff] [review]
getcert.cgi

Pushed to production:
https://hg.mozilla.org/build/puppet/rev/abdef7a1719f

Comment 13

[Rail Aliiev [:rail]]

Comment on attachment 8338579 [details] [diff] [review]
getcert.cgi

Backed out for 2 reasons:
1) needs a secreview
2) issues in relabs (no manager_ips, etc set)


remote:   https://hg.mozilla.org/build/puppet/rev/a722787ded87
remote:   https://hg.mozilla.org/build/puppet/rev/943cde65d26c

Comment 14

[Joe Stevensen [:joe]]

Bumping priority per Taras' request.

Comment 15

[Michael Henry [:tinfoil]]

Starting the sec-review at 12:00 PST today.

Comment 16

[Michael Henry [:tinfoil]]

Just a status update I've cloned the repository and am looking at the puppetmaster module now, specifically the getcert.cgi.erb and related manifests

Comment 17

[Michael Henry [:tinfoil]]

Finding 1:

In modules/puppetmaster/manifests/deploy.pp change
         "${puppetmaster::settings::deploy_dir}/cgi-bin":
            ensure => directory;

to change folder permissions to root:apache 0750 to ensure the folder is not writable by the apache process.

Comment 18

[Rail Aliiev [:rail]]

Attachment: safer permissions

(In reply to Michael Henry [:tinfoil] from comment #17)
> Finding 1:
> 
> In modules/puppetmaster/manifests/deploy.pp change
>          "${puppetmaster::settings::deploy_dir}/cgi-bin":
>             ensure => directory;
> 
> to change folder permissions to root:apache 0750 to ensure the folder is not
> writable by the apache process.

Thanks for the fast turn around! The patch should address this comment.

Comment 19

[Michael Henry [:tinfoil]]

Finding 2:

I'd recommend making the cipher suites and ordering match our guidelines:

https://wiki.mozilla.org/Security/Server_Side_TLS#Apache

This isn't a requirement, but a recommendation.

-----------

My one area for minor concern is perhaps filtering the hostname to ensure it's file-system safe.  That can be done with proper regex supplied via puppet, or you can add it to the script for redundancy

I didn't find any faults with getcert.cgi.  It's deliberately kept simple.  It's untrusted inputs are kept simple and filtered.  The access restrictions on the script make sense.

Comment 20

[Rail Aliiev [:rail]]

Comment on attachment 8342708 [details] [diff] [review]
safer permissions

I'll integrate this into the final patch

Comment 21

[Rail Aliiev [:rail]]

Thanks a lot for the review.

(In reply to Michael Henry [:tinfoil] from comment #19)
> My one area for minor concern is perhaps filtering the hostname to ensure
> it's file-system safe.  That can be done with proper regex supplied via
> puppet, or you can add it to the script for redundancy

Can you elaborate on this? The script has the following check:

die "refused FQDN: $host" unless grep $host =~ /$_/, @$fqdn_regexps;                   

ATM we check against /mozilla\.com/ regexp, but we can make it stricter.

Comment 22

[Michael Henry [:tinfoil]]

Mostly I wasn't sure if DNS potentially allowed characters that might have unintended consequences when passed to an external program or passed into a file name.  That being said, I took a look and it seems like it should be okay.

Comment 24

[Rail Aliiev [:rail]]

Comment on attachment 8338579 [details] [diff] [review]
getcert.cgi

https://hg.mozilla.org/build/puppet/rev/8e2d336ff8d5

Comment 25

[Rail Aliiev [:rail]]

Comment on attachment 8342708 [details] [diff] [review]
safer permissions

https://hg.mozilla.org/build/puppet/rev/8e2d336ff8d5

Comment 26

[Rail Aliiev [:rail]]

in production https://hg.mozilla.org/build/puppet/rev/7ab7d791de6e

Comment 27

[Rail Aliiev [:rail]]

Attachment: gen.diff

Everything worked fine except the first line in the puppet-change emails:

Creating certificate for tst-linux64-spot-116.test.releng.use1.mozilla.com from ip 10.134.56.50

It should mention the IP address of the machine generated it.

Comment 28

[Rail Aliiev [:rail]]

Comment on attachment 8343510 [details] [diff] [review]
gen.diff

remote:   https://hg.mozilla.org/build/puppet/rev/6324036b2e08
remote:   https://hg.mozilla.org/build/puppet/rev/109e88f4f8df