search

schubergphilis / chef-acme

Chef cookbook to request SSL certificates at Let's Encrypt
Apache License 2.0
112 stars 74 forks source link

Use of sensitive property to avoid logging or report of certificates #123

Closed djessich closed 4 years ago

djessich commented 4 years ago

Problem Description

Using the acme cookbook to generate Letsencrypt certificates works as desired, but the generated certificates will be printed to logs and reported to the Chef Server. This represents a security problem and can be avoided by properly using the sensitive resource property on all resources.

When does it happen?

Every time a new certificate was generated.

Reproduction

acme_selfsigned node['fqdn'] do
  key "#{nginx_dir}/certs/#{node['fqdn']}.key"
  crt "#{nginx_dir}/certs/#{node['fqdn']}.crt"
  owner 'root'
  group node['root_group']
end

nginx_site node['fqdn'] do
  # ...
end

acme_certificate node['fqdn'] do # <-- prints sensitive content to log
  key "#{nginx_dir}/certs/#{node['fqdn']}.key"
  crt "#{nginx_dir}/certs/#{node['fqdn']}.crt"
  owner 'root'
  group node['root_group']
end

Chef Client Log (from Kitchen test VM)

* acme_certificate[example.com] action create
  * file[example.com SSL key] action create_if_missing (up to date)
  * directory[/var/www/.well-known/acme-challenge] action create
    - create new directory /var/www/.well-known/acme-challenge
    - change mode from '' to '0755'
    - change owner from '' to 'root'
    - change group from '' to 'root'
  * file[/var/www/.well-known/acme-challenge/MlnzeLIk3TUqmNyO-j1VXQch0rg5h7wTdOMY8usxGGo] action create
    - create new file /var/www/.well-known/acme-challenge/MlnzeLIk3TUqmNyO-j1VXQch0rg5h7wTdOMY8usxGGo
    - update content in file /var/www/.well-known/acme-challenge/MlnzeLIk3TUqmNyO-j1VXQch0rg5h7wTdOMY8usxGGo from none to 24b1d4
    --- /var/www/.well-known/acme-challenge/MlnzeLIk3TUqmNyO-j1VXQch0rg5h7wTdOMY8usxGGo 2020-08-18 17:01:57.809701369 +0000
    +++ /var/www/.well-known/acme-challenge/.chef-MlnzeLIk3TUqmNyO-j1VXQch0rg5h7wTdOMY8usxGGo20200818-110-1qk7h81   2020-08-18 17:01:57.809701369 +0000
    @@ -1 +1,2 @@
    +MlnzeLIk3TUqmNyO-j1VXQch0rg5h7wTdOMY8usxGGo.9pnruov_H9rVLQSKd5g3J1Yyr5TxE4gf08jW9ydiSC4
    - change mode from '' to '0644'
    - change owner from '' to 'root'
    - change group from '' to 'root'
  * file[example.com SSL key] action nothing (skipped due to action :nothing)
  * directory[/var/www/.well-known/acme-challenge] action nothing (skipped due to action :nothing)
  * file[/var/www/.well-known/acme-challenge/MlnzeLIk3TUqmNyO-j1VXQch0rg5h7wTdOMY8usxGGo] action nothing (skipped due to action :nothing)
  * file[/var/www/.well-known/acme-challenge/MlnzeLIk3TUqmNyO-j1VXQch0rg5h7wTdOMY8usxGGo] action delete
    - delete file /var/www/.well-known/acme-challenge/MlnzeLIk3TUqmNyO-j1VXQch0rg5h7wTdOMY8usxGGo
  * ruby_block[create certificate for example.com] action run
  Recipe: <Dynamically Defined Resource>
    * file[example.com SSL new crt] action create
      - update content in file /etc/nginx/certs/example.com.crt from c3d8b5 to b83bf9
      --- /etc/nginx/certs/example.com.crt  2020-08-18 17:01:56.809711906 +0000
      +++ /etc/nginx/certs/.chef-20200818-110-1q60n6w.example.com.crt   2020-08-18 17:01:59.901679324 +0000
      @@ -1,30 +1,47 @@
       -----BEGIN CERTIFICATE-----
      -MIIE4DCCAsigAwIBAgIBADANBgkqhkiG9w0BAQsFADAbMRkwFwYDVQQDDBBqaXJh
      ...
      -CCEwRo4CxL0L9yeGvt+p61DUKCI2ZSZq27ZBCiIid5EuIoLynkLaCEQb9nKAD3dp
      -iyVW9g==
      +MIIEYzCCA0ugAwIBAgIIf+JSA49tNqEwDQYJKoZIhvcNAQELBQAwKDEmMCQGA1UE
      ...
      +yxwWolFslOPe6X2aWzfeB+q5+vP/0+B21mQts9/hyMnK3xg2KZfJ+Z/+F/eEpCDf
      +Etz5l2HDmaBta4WgBblQsEvWhmDpeXU=
      +-----END CERTIFICATE-----
      +-----BEGIN CERTIFICATE-----
      +MIIDUDCCAjigAwIBAgIIQeCtBbzwiSMwDQYJKoZIhvcNAQELBQAwIDEeMBwGA1UE
      ...
      +iSMFSteu3JZ9K2M5uL+cr+tSa8ryjAy8ldzxpH7BdeCsn26LxbVhpVQE6R5iw5Oh
      +xE2aoBq4NxUKfeUK7LLfV1Hknk7GGVRJQAd7dcAQ45Scsaz1
       -----END CERTIFICATE-----
    - execute the ruby block create certificate for example.com

Note: I removed some parts from the auto generated certificates to reduce the log size and to avoid overfilling this issue.

The actual problematic output is:

Recipe: <Dynamically Defined Resource>
    * file[example.com SSL new crt] action create

Environment

Chef: 15.13.8+ acme Cookbook: 4.1.2

Possible Workaround

Currently none, as this needs to be handled in the acme cookbook.

Possible Implementation

A possible implementation may be as follows:

ruby_block "create certificate for #{new_resource.cn}" do # ~FC014
  block do
    unless (all_validations.map { |authz| authz.status == 'valid' }).all?
      fail "[#{new_resource.cn}] Validation failed, unable to request certificate"
    end

    begin
      newcert = acme_cert(order, new_resource.cn, mykey, new_resource.alt_names)
    rescue Acme::Client::Error => e
      fail "[#{new_resource.cn}] Certificate request failed: #{e.message}"
    else
      Chef::Resource::File.new("#{new_resource.cn} SSL new crt", run_context).tap do |f|
        f.path    new_resource.crt
        f.owner   new_resource.owner
        f.group   new_resource.group
        f.content newcert
        f.mode    00644
        f.sensitive true # <-- FIX (may also be set with value: new_resource.sensitive)
      end.run_action :create
    end
  end
end

(see https://github.com/schubergphilis/chef-acme/blob/master/resources/certificate.rb#L108)

It should be well thought, where the sensitive property makes sense in all the resources provided by this cookbook. I could think of the acme challenge token file creation and the public certificate (as I think that no certificate, regardless of its type, should be reported). Maybe there are more places where the use of sensitive property makes sense.

However, the above implementation example will avoid the report problem of the generated certificate.

thoutenbos commented 4 years ago

Thanks for the extensive report!

In PKI the certificate itself it not a sensitive object, so for security it does not matter. It's even sent to every client when a connection is negotiated. Nonetheless marking it sensitive in the resource would not harm, so I'd be okay to accept a pull request.

djessich commented 4 years ago

@thoutenbos Yes, of course you're right. I oversaw, that the dynamically created resource is just for the public crt certificate, but I thought its for the private key. Therefore, I will close this issue, as the file resource for the private key within acme_certificate is defined as being sensitive anyway.