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

Fixes #23211 - Add PuppetCa TokenWhitelisting provider #592

Merged
merged 1 commit into from Aug 27, 2018

Conversation

Projects
None yet
4 participants
@juliantodt
Member

juliantodt commented Jun 21, 2018

This adds an alternative PuppetCa provider.
Instead of signing CSRs based on whitelist of hosts (directly via puppet), we will create a signed JWT and return it to foreman that will provision the token on the host so that it is included in the CSR the hosts sends to the PuppetCa. The PuppetCa will then use a script (rather than the textfile) (see puppet-foreman_proxy) that calls the SmartProxy with the CSR that will then validate the token inside it. This should be much more secure and easier to debug than the current hostname_whitelisting provider.
Note: For this to work, you will need #5730 in your foreman instance.

@juliantodt juliantodt changed the title from Fixes #23211 - Add PuppetCa TokenVerify provider to Fixes #23211 - Add PuppetCa TokenWhitelisting provider Jun 21, 2018

@ekohl

How do we prevent concurrency issues with writing to the hosts file?

# Create a new token for a certname
def autosign certname
ensure_hostsfile
payload = { certname: certname, exp: Time.now.to_i + 10 * 60 * 60 }

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

Perhaps this time should be configurable. IMHO at least defined as a constant so you know what this magic value is.

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

Yes, I agree. Do you think 10 hours is a good default?

This comment has been minimized.

@timogoebel

timogoebel Jun 25, 2018

Member

Currently we allow 6 hours in foreman core as a default (for build tokens). I'd suggest do use the same.

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

If Foreman has a setting for this, should we have it as an API parameter instead? You could still apply a default there but that way you would have a single setting.

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

That would mean that the API would be different for the providers, right?

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

I think an optional API parameter for the TTL would still be compatible enough. You may want to raise a 400 Bad Request on the old provider if a TTL is provided.

This comment has been minimized.

@juliantodt

juliantodt Jun 27, 2018

Member

Raising an 400 on the old provider isn't a good idea, as foreman doesnt know which provider is used and will (if we add that) always include the TTL, meaning the old provider would not work. Will add the optional parameter.

This comment has been minimized.

@ekohl

ekohl Jun 27, 2018

Member

That's a fair point. #585 is my take at solving this but I don't see this being integrated any time soon.

payload = { certname: certname, exp: Time.now.to_i + 10 * 60 * 60 }
token = JWT.encode payload, smartproxy_cert, 'RS512'
File.write hosts_file, autosign_list.push(certname).to_yaml
{ generated_token: token }.to_json

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

Should the JSON representation be done in this function? IMHO it's something that you do in the API class

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

Will move.

This comment has been minimized.

@juliantodt

juliantodt Jun 27, 2018

Member

Moving the .to_json to the API class would mean that we might change the behaviour of the old provider.

def ensure_hostsfile
unless File.exist?(hosts_file)
FileUtils.mkdir_p hosts_file.rpartition('/').first

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

Isn't the rpartition('/').first equivalent to dirname()?

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

Yeah, sometimes I just remember the complicated ways to do thinks. Will change this.

plugin :puppetca_token_whitelisting, ::Proxy::VERSION
requires :puppetca, ::Proxy::VERSION
default_settings :sign_all => false, :hosts_file => '/tmp/foreman-proxy/hosts.yml'

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

I don't like this default because /tmp is a shared directory. Another user on the system can create /tmp/foreman-proxy prior to startup and have control. That makes it a security risk. We do have /var/lib/foreman-proxy in some packages (notably REX/Ansible I think) and perhaps we should standardize this. An alternative is doing it in /etc/foreman-proxy but I don't like it because it gives the impression users can/should modify it.

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

We also weren't sure where to put the file. Its not code, it's not config, it does not belong in the puppet dir. I settled for this as it is a temporary file, but am not quite happy, so I'm open to suggestions. You think /var/lib/foreman-proxy/ would be the best idea?

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

/tmp is wiped after a reboot. Losing that kind of state doesn't sound right for this file. /var/tmp would be better but to me it sounds like this is actually a state file. For that /var/lib is generally used.

include ::Proxy::Util
def sign_all
Proxy::PuppetCa::TokenWhitelisting::Plugin.settings.sign_all

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

I am still wondering if it should allow reading a whitelist from the actual autosign file but this might be enough.

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

I think falling back to the autosign.conf could be confusing. Allowing this for people who really really want to put the * in the file should be fine.

def autosign certname
ensure_hostsfile
payload = { certname: certname, exp: Time.now.to_i + 10 * 60 * 60 }
token = JWT.encode payload, smartproxy_cert, 'RS512'

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

I think the RS512 algorithm should be stored in a constant.

unless File.exist?(hosts_file)
FileUtils.mkdir_p hosts_file.rpartition('/').first
FileUtils.touch hosts_file
File.write(hosts_file, [].to_yaml)

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

I'd move writing to this file to a (private) helper function rather than duplicate it in 3 places.

# List the hosts that are currently valid
def autosign_list
ensure_hostsfile
YAML.load_file(hosts_file).to_a

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

Should safe_load be used? Especially if the default location is in /tmp there's a risk of executing arbitrary code as the foreman-proxy user as long as you can write anything in /tmp. Because of it I'd actually consider JSON rather than YAML. Since no user has to actually modify the file, it doesn't matter that it's not very human friendly.

@timogoebel

@juliantodt: Thanks, I left some comments inline.

post "/validate" do
content_type :json
unless autosigner.respond_to?('validate_csr')
log_halt 401, "Provider only supports trivial autosigning"

This comment has been minimized.

@timogoebel

timogoebel Jun 25, 2018

Member

401 is unauthorized, how about 406 or 412?

This comment has been minimized.

@ekohl

ekohl Jun 25, 2018

Member

406 would be incorrect because that's on Accept headers. I'd consider HTTP 501 Not Implemented.

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

I was thinking Bad Request but that would have been 400. 501 would be fine for me.

return false
end
if sign_all
logger.warn "Signing CSR without token verification."

This comment has been minimized.

@timogoebel

timogoebel Jun 25, 2018

Member

Should be info I think.

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

We had that when we talked about the first SmartProxyPR #576 where witlessbird suggested warnings.

def disable certname
hosts = autosign_list
hosts.delete(certname)
File.write hosts_file, hosts.to_yaml

This comment has been minimized.

@timogoebel

timogoebel Jun 25, 2018

Member

I think we should extract this to a dedicated storage class.

end
def smartproxy_cert
OpenSSL::PKey::RSA.new File.read Proxy::SETTINGS.ssl_private_key.to_s

This comment has been minimized.

@timogoebel

timogoebel Jun 25, 2018

Member

What happens if the smart proxy is run without ssl?

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

Well, that would be a problem. I guess we would have to ensure that there is a RSA certificate there if used for SSL or not...

# Invalidate a token based on the certname
def disable certname
hosts = autosign_list

This comment has been minimized.

@timogoebel

timogoebel Jun 25, 2018

Member

I believe we should store the (in)valid tokens here instead of the certnames. If a token is lost, a user has no way of actually revoking it for good.

This comment has been minimized.

@juliantodt

juliantodt Jun 25, 2018

Member

Agreed, I think storing valid tokens is the way to go. It allows us to do everything we do now (since we can always extract the certname from the tokens) while we still don't have to clean up periodically because the list would auto-clean itself + additionally we can revoke specific tokens manually.

end
@autosigner = Proxy::PuppetCa::TokenWhitelisting::Autosigner.new
@autosigner.stubs(:hosts_file).returns(@file.path)
rsa_cert = OpenSSL::PKey::RSA.generate 2048

This comment has been minimized.

@ekohl

ekohl Jun 27, 2018

Member

Does this mean a RSA cert is generated for every test in this file? Isn't that somewhat slow?

This comment has been minimized.

@juliantodt

juliantodt Jun 27, 2018

Member

Not noticeable (at least for me). But since we decided to change the implementation to save the tokens instead of certnames, we will need to supply a fixtured cert anyway.

@juliantodt

This comment has been minimized.

Member

juliantodt commented Jun 27, 2018

Updated:

  • Saving tokens instead of certnames
  • Moving TokenStorage to external class
  • Locking file during read/writing
  • Configurable TTL
  • Moved default location of tokens file

TODO:

  • Add tests for storage class
  • Handle case where SmartProxy doesnt use SSL
  • Fix tests, rubocop ^^
@juliantodt

This comment has been minimized.

Member

juliantodt commented Jun 28, 2018

Updated. This should now satisfy all your concerns. Thanks @ekohl @timogoebel! Tests are now green as well.

Question: If we decide to add the token ttl to the foreman api call, we would break compatibility with old SmartProxy versions as they would not recognise the url, right?

@timogoebel

This comment has been minimized.

Member

timogoebel commented Jun 28, 2018

Question: If we decide to add the token ttl to the foreman api call, we would break compatibility with old SmartProxy versions as they would not recognise the url, right?

I think we can live with that. We could check the smart proxy version in foreman and add a switch for that. Or just try again if we get a 404. @ekohl: WDYT?

content_type :json
certname = params[:certname]
certname = params['captures'][0]
ttl = params['captures'][2]

This comment has been minimized.

@ekohl

ekohl Jun 29, 2018

Member

Isn't the TTL more of a body parameter in REST?

This comment has been minimized.

@timogoebel

timogoebel Jun 29, 2018

Member

Yeah, we could make this a parameter and not part of the URL. That would make this even more backward compatible.

rescue JWT::ExpiredSignature
nil
end
end.reject { |certname| certname.nil? }

This comment has been minimized.

@ekohl

ekohl Jun 29, 2018

Member

Isn't this equal reject equal to compact?

@timogoebel

@juliantodt: Yet another round of comments. :-)

@@ -0,0 +1,7 @@
group :puppetca_token_whitelisting do
if RUBY_VERSION < '2.1'
gem 'jwt', '1.5.6'

This comment has been minimized.

@timogoebel

timogoebel Jun 29, 2018

Member

Can we use ~> 1.5.6?

rescue => e
log_halt 406, "Failed to enable autosign for #{certname}: #{e}"
end
end
post "/validate" do
content_type :json
unless autosigner.respond_to?('validate_csr')

This comment has been minimized.

@timogoebel

timogoebel Jun 29, 2018

Member

Nit: I'd us a symbol here: respond_to?(:validate_csr)

end
def ensure_file
unless File.exist?(@tokens_file)

This comment has been minimized.

@timogoebel

timogoebel Jun 29, 2018

Member

you can use a guard clause here, return of File.exists?(@token_file)

end
end
def locked_write content

This comment has been minimized.

@timogoebel

timogoebel Jun 29, 2018

Member

I think the method name is misleading, as the write is not actually locked here.

@juliantodt

This comment has been minimized.

Member

juliantodt commented Jul 2, 2018

Updated based on your comments, thank you!
Token TTL is now a body parameter which should make the foreman implementation backwards-compatible.

file = Proxy::PuppetCa::TokenWhitelisting::Plugin.settings.certificate
unless File.exist?(file)
File.write file, OpenSSL::PKey::RSA.generate(2048)

This comment has been minimized.

@ekohl

ekohl Jul 2, 2018

Member

It's probably good to do this with a umask so it's not world readable (or some other way). The number of bits should also be at least a constant IMHO.

This comment has been minimized.

@juliantodt
end
end
def unsave_write content

This comment has been minimized.

@ekohl

ekohl Jul 2, 2018

Member

unsafe?

This comment has been minimized.

@juliantodt
true
end
end
storage.unsave_write tokens

This comment has been minimized.

@ekohl

ekohl Jul 2, 2018

Member

Any reason this should be unsafe?

This comment has been minimized.

@juliantodt

juliantodt Jul 2, 2018

Member

The whole block is already locked so nothing changes the file between read and write. You would not be able to safely write here, as that would try to lock the file again which is already locked.

This comment has been minimized.

@ekohl

ekohl Jul 2, 2018

Member

I wonder if the storage should have an operation where you pass in a block to delete tokens. It feels a bit like you're doing storage operations in the signer here.

This comment has been minimized.

@juliantodt

juliantodt Jul 4, 2018

Member

Added a TokenStorage::remove_if method, take a look.

@juliantodt

This comment has been minimized.

Member

juliantodt commented Jul 27, 2018

As requested, I moved the puppet_sign script from the Puppet-Foreman_Proxy PR to here and added a couple of improvements.
@ekohl Is extra/puppet_sign.rb the correct place for it?

@timogoebel

This comment has been minimized.

Member

timogoebel commented Aug 24, 2018

@ekohl: Any further comments?

@ekohl

Overall it looks good and I'd be OK with merging. Just small edge cases you might want to look at.

end
SETTINGS = YAML.load_file(settings_file)
PUPPETCA = YAML.load_file(SETTINGS[:settings_directory] + '/puppetca.yml')
protocol = PUPPETCA[:enabled] == true ? 'https' : PUPPETCA[:enabled]

This comment has been minimized.

@ekohl

ekohl Aug 24, 2018

Member

Probably good if this also handles the value false and print a proper error

This comment has been minimized.

@juliantodt
end
begin
request.body.rewind
autosigner.validate_csr(request.body.read) ? 200 : 404

This comment has been minimized.

@ekohl

ekohl Aug 24, 2018

Member

I'm wondering about the 404 code; will there be a need to distinguish between a proxy without this feature and one with? Probably not but it might be worth to consider 400 Bad Request for invalid CSRs.

This comment has been minimized.

@juliantodt

juliantodt Aug 27, 2018

Member

Well the 404 here only gets returned when the CSR is valid but the token is not. A proxy without this feature returns 501 (line 43) and an invalid CSR raises an Exception leading to a 406 (line 49). Does that work for you?

@timogoebel

This comment has been minimized.

Member

timogoebel commented Aug 27, 2018

@ekohl: Final ACK? You still "request changes" on this PR. :-)

@ekohl

ekohl approved these changes Aug 27, 2018

@timogoebel timogoebel merged commit 124af3d into theforeman:develop Aug 27, 2018

3 checks passed

core Build finished. 5364 tests run, 0 skipped, 0 failed.
Details
prprocessor Commit message style is correct
Details
rubocop Build finished. No test results found.
Details
@timogoebel

This comment has been minimized.

Member

timogoebel commented Aug 27, 2018

Thanks, @juliantodt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment