Ok, that helps me to understand the concern, thanks. Now one thing I still don’t understand. How I can take advantage of the UUID if I don’t know the oauth and I can’t add the signature (and confirm, I’m the proxy who can register this UUID). Note that the signature is not something that would be automatically added to every fact upload coming from other hosts (or at least that was my understanding). Foreman would take the UUID fact into consideration only if the signature in the same fact set is valid.
Now my assumption is, the signature depends on more things than just the UUID value. Therefore it can’t be replayed. If that’s not the case, I see the problem of just stealing both facts and using it from another host.
If there is something that makes the UUID secret or if there’s a way to abuse it without knowing the oauth secret, then I agree, it should not be sent as a fact and I’d like your proposal more.
That perhaps depends on the signature implementation. If the signature takes also the timestamp into the signed data, you can’t take it and reuse the signature elsewhere. I think we agreed on using HMAC-SHA256, so if we sha256(oauth_secret + _timestamp + uuid) and on the Foreman side we verify the same, the only secret is the oauth_secret. That however is not send as any fact. Of course Foreman would need to have a few minutes time window to compare the incoming facts _timestamp and the time of the actual request.
If I’m not mistaken, the step 3 of your attack vector is mitigated by the above. There’s no way of injecting the instance_id fact to non-proxy host facts without knowing the oauth_secret. That is typically only stored in the proxy installer. If attacker gets access to it, that’s of course a problem, but a problem we already have today.
Correct, if we would add a time window, this would be eliminated. But I’d very much prefer if we would not design yet another signing scheme here – JWT exists and supports expiration.
But I also like Ewouds proposal with a simple API that doesn’t have those problems to begin with.
Alright, so to summarize there’s no security concern with the original proposal. The alternative with explicit endpoints seems more straightforward.
The con I see with the alternative is, the host must exist before the installer is ran. To mitigate that, it was suggested to also send facts so we can also create the host. To me that feels as a drawback because of duplicate the facts import logic, so I’d vote for the original proposal. What we use as a signature mechanism isn’t that important to me (JWT or whatever else) as long as it’s based on the same secret we use in the installer today - oauth_secret. Having said that I can live with additional endpoints too.
@aruzicka thoughts? Would it make sense to do the poll? Do you lean strongly towards one of these? At the end you’re implementing it, so the final decision is up to you.
For what it’s worth, I thought it was a nice optimization but perhaps it’s not needed. A different flow could be:
GET /api/smart_proxies/:smart_proxy_id/hosts
This retrieves the list of hosts. Each entry must at least have the host ID and certname fields.
Then the installer code can check if its certname is within that list. If it’s not, it can add it. So far, it’s the same as my previous proposal.
POST /api/smart_proxies/:smart_proxy_id/hosts
This is where it becomes interesting. What do you post. Do you post just a host ID or certname? If posting a certname, the host may not exist.
If the host does not exist, you can return a specific error code and let the client create a host if needed and then post again. My suggestion to also accept facts was to remove the separate call.
You could perhaps even implement PUT /api/smart_proxies/:smart_proxy_id/hosts/:host_id where the host ID could also be a certname.
Now the reason I suggest this explicit flow over facts is that there is a plan to make Puppet optional. Fact upload currently relies on Puppet.
Ok, that makes more sense to me. This is not the operation that would be done too often, so I’d be fine with multiple requests.
This is important point. Not because puppet is becoming optional, for that, Adam shared the plan earlier Infrastructure roles - #34 by aruzicka. Fact importing is universal and we can implement the same thing for all 5 fact sources (puppet, rhsm, ansible, salt, chef). But the drawback is it would always be a different implementation which is a maintenance burden. That is a benefit of the alternative I didn’t consider earlier.
At the same time the alternative seems to only rely on the case, where the endpoint is called from the installer. If user deleted and recreates the host (eg by new facts upload), it won’t ever be re-linked. We need the installer run for that. This is likely an edge case. If we later add also explicit association capability to the user, it’s resolved.
I rewrote the whole thing from scratch using explicit api. There are two new sets of three API endpoints (index, create, destroy). One set is for foreman itself, the other is for smart proxies.
On the installer side this adds three new types (and providers):
foreman_host - a host in foreman
foreman_instance_host - marking an existing host as foreman instance
foreman_smartproxy_host - marking an existing host as a smart proxy
