Bug 1467898

Summary: [RFE] Verify keys when configuring
Product: [oVirt] ovirt-engine-metrics Reporter: Yedidyah Bar David <didi>
Component: GenericAssignee: Shirly Radco <sradco>
Status: CLOSED DEFERRED QA Contact: Lukas Svaty <lsvaty>
Severity: medium Docs Contact:
Priority: unspecified    
Version: unspecifiedCC: bugs, mperina, sradco
Target Milestone: ---Keywords: FutureFeature
Target Release: ---Flags: oourfali: ovirt-future?
rule-engine: planning_ack?
rule-engine: devel_ack?
rule-engine: testing_ack?
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2020-04-01 14:46:01 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: Infra RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1490404    
Bug Blocks:    

Description Yedidyah Bar David 2017-07-05 12:49:30 UTC
Description of problem:

For bug 1438821 we made ansible not check host keys.

We should fix this, to make the process more secure.

Currently, the engine keeps in the database the fingerprint of each host's public key, but not the key itself.

OpenSSH seems to require the full key. It also has an option VerifyHostKeyDNS to verify against the DNS, where it's enough to have the fingerprint.

Some ideas/thoughts about how to check:

1. Set up a private DNS server with the fingerprints and point ssh to verify against it. Not very likely to be done.

2. Verify using the fingerprint directly. E.g. something like this:
- Keep the output of: ssh -o StrictHostKeyChecking=Yes -v $MACHINE
- Parse it to find the fingerprint, looks like this:
debug1: Server host key: ECDSA 37:9c:8b:75:9f:75:6e:90:d6:9b:c1:ab:05:fc:39:01
- If it matches the fingerprint in the DB, use ssh-scankey to get the key and save it in known_hosts.
- Perhaps have our own known_hosts maintained somewhere in /var.
- Probably need to patch ansible to do all of above, but in principle can be done beforehand.

3. Make the engine save also the full public key in the db and get it from there, save to (our own?) known_hosts. Will need to do this also for existing hosts somehow.

4. Patch OpenSSH to allow verification by fingerprint and/or by some pluggable method.

5. OpenSSH also allows using a CA to sign keys and then trust them based on the signature, similarly to PKI. Didn't look at how it works.

Comment 1 Yedidyah Bar David 2017-07-06 06:45:06 UTC
Before the solution to bug 1438821, we did verify keys. It probably wasn't comfortable, but we did. So one can claim that current bug is a regression, not an RFE. If we decide to postpone it to some undefined far future, I'd prefer to revert bug 1438821, and keep the behavior less comfortable but more secure, suggesting some manual workaround instead for those that prefer the opposite.

When adding a host, we do allow providing or fetching the host's public key fingerprint for validation.

Comment 2 Oved Ourfali 2017-07-06 06:50:39 UTC
Shirly - your input?

Comment 3 Yaniv Lavi 2017-08-30 13:32:11 UTC
I sound to me that the engine needs to make sure the keys are valid.
Yaniv K, what do you think?

Comment 4 Yaniv Kaul 2017-08-30 15:37:05 UTC
(In reply to Yaniv Lavi (Dary) from comment #3)
> I sound to me that the engine needs to make sure the keys are valid.
> Yaniv K, what do you think?

Indeed, would be nice. But AFAIK we have not done it previously.
How is it done elsewhere when working with Ansible?

Comment 5 Martin Perina 2017-08-31 11:57:56 UTC
(In reply to Yaniv Kaul from comment #4)
> (In reply to Yaniv Lavi (Dary) from comment #3)
> > I sound to me that the engine needs to make sure the keys are valid.
> > Yaniv K, what do you think?
> 
> Indeed, would be nice. But AFAIK we have not done it previously.
> How is it done elsewhere when working with Ansible?

AFAIK by default Ansible don't care about host fingerprints, we just pass engine private key /etc/pki/ovirt-engine/keys/engine_id_rsa to Ansible and it connects to the host wihtout any issue.

Regarding storing SSH public key instead of fingerprint, Alon tried to pushed that in https://gerrit.ovirt.org/50226 but it didn't get in as it broke V3 API because of removing fingerprint. So if needed we can finish it up although not sure if we can make it till 4.2 feature freeze.

Comment 6 Yedidyah Bar David 2017-09-03 10:06:40 UTC
(In reply to Martin Perina from comment #5)
> (In reply to Yaniv Kaul from comment #4)
> > (In reply to Yaniv Lavi (Dary) from comment #3)
> > > I sound to me that the engine needs to make sure the keys are valid.
> > > Yaniv K, what do you think?
> > 
> > Indeed, would be nice. But AFAIK we have not done it previously.
> > How is it done elsewhere when working with Ansible?

No idea, in general.

> 
> AFAIK by default Ansible don't care about host fingerprints, we just pass
> engine private key /etc/pki/ovirt-engine/keys/engine_id_rsa to Ansible and
> it connects to the host wihtout any issue.

AFAIU (and see) ansible by default runs ssh more-or-less like a user would.
This means that the server's (ovirt hosts, in our case) public key is looked up at ~/.ssh/known_hosts (where '~' is /root, as we need root to access the engine db without credentials), and if not found, ssh (and ansible) prompt, asking the user to confirm the key.

> 
> Regarding storing SSH public key instead of fingerprint,

Must it be instead? Can't we keep both? I do not think keeping also the fingerprint will be such a big waste, hardware-resources-wise. And dropping it might in principle be a big waste of human resources (for whoever that needs to adapt existing code to the new api).

> Alon tried to
> pushed that in https://gerrit.ovirt.org/50226 but it didn't get in as it
> broke V3 API because of removing fingerprint. So if needed we can finish it
> up although not sure if we can make it till 4.2 feature freeze.

The commit message explicitly says it's breaking restapi compat as it was targetted 4.0. Perhaps we can reconsider, either keeping both or making the engine extract the fingerprint out of the public key, for api compat - but I suspect it would be simpler to keep both, and probably less wasteful - assuming this data is read much more than it's written.

Comment 7 Martin Perina 2017-09-11 11:45:25 UTC
(In reply to Yedidyah Bar David from comment #6)
> (In reply to Martin Perina from comment #5)
> > (In reply to Yaniv Kaul from comment #4)
> > > (In reply to Yaniv Lavi (Dary) from comment #3)
> > > > I sound to me that the engine needs to make sure the keys are valid.
> > > > Yaniv K, what do you think?
> > > 
> > > Indeed, would be nice. But AFAIK we have not done it previously.
> > > How is it done elsewhere when working with Ansible?
> 
> No idea, in general.
> 
> > 
> > AFAIK by default Ansible don't care about host fingerprints, we just pass
> > engine private key /etc/pki/ovirt-engine/keys/engine_id_rsa to Ansible and
> > it connects to the host wihtout any issue.
> 
> AFAIU (and see) ansible by default runs ssh more-or-less like a user would.
> This means that the server's (ovirt hosts, in our case) public key is looked
> up at ~/.ssh/known_hosts (where '~' is /root, as we need root to access the
> engine db without credentials), and if not found, ssh (and ansible) prompt,
> asking the user to confirm the key.
> 
> > 
> > Regarding storing SSH public key instead of fingerprint,
> 
> Must it be instead? Can't we keep both? I do not think keeping also the
> fingerprint will be such a big waste, hardware-resources-wise. And dropping
> it might in principle be a big waste of human resources (for whoever that
> needs to adapt existing code to the new api).

AFAIK there's no issue to add public key to RESTAPI (and generate fingerprint for it), but we cannot remove using figerprint only (without public key), because we would break the API. And that was the main reason why Alon failed, because we still need to support both APIv3 and APIv4. But if needed we can reopen the discussion and see what we can do ...

> 
> > Alon tried to
> > pushed that in https://gerrit.ovirt.org/50226 but it didn't get in as it
> > broke V3 API because of removing fingerprint. So if needed we can finish it
> > up although not sure if we can make it till 4.2 feature freeze.
> 
> The commit message explicitly says it's breaking restapi compat as it was
> targetted 4.0. Perhaps we can reconsider, either keeping both or making the
> engine extract the fingerprint out of the public key, for api compat - but I
> suspect it would be simpler to keep both, and probably less wasteful -
> assuming this data is read much more than it's written.

So if required we can start discussing it, but I don't think we will be able to finish this one till oVirt 4.2 feature freeze, but it could be possible for some 4.2.z (assuming we will get to the agreement around "breaking the API"

Comment 8 Shirly Radco 2018-09-04 12:56:03 UTC
I think we close this bug.

Comment 9 Yaniv Lavi 2018-09-05 09:35:04 UTC
(In reply to Shirly Radco from comment #8)
> I think we close this bug.

Please discuss the security implications with Didi.

Comment 10 Yedidyah Bar David 2018-09-05 09:52:34 UTC
I do not see why this bug is now less critical than when I filed it.

The security implications are obvious, and are not related to metrics, or even to oVirt. For examples, search the net for things like "StrictHostKeyChecking insecure".

See also comment 1.

The main reason this is less critical than the general case, is that we can probably assume that hosts connected to by metrics are also regularly used by the engine, and can hope that if something as bad as e.g. someone impersonating a host machine happens, users will notice this quickly due to other problems, hopefully evident also on the engine (e.g. VMs disappearing, stuff like that).

Comment 11 Shirly Radco 2018-09-05 10:38:32 UTC
I missed the  dependent bug for this, that is targeted to 4.4.
Will keep it open. Thanks.

Comment 12 Michal Skrivanek 2020-03-18 15:44:59 UTC
This bug didn't get any attention for a while, we didn't have the capacity to make any progress. If you deeply care about it or want to work on it please assign/target accordingly

Comment 13 Michal Skrivanek 2020-03-18 15:50:01 UTC
This bug didn't get any attention for a while, we didn't have the capacity to make any progress. If you deeply care about it or want to work on it please assign/target accordingly

Comment 14 Michal Skrivanek 2020-04-01 14:46:01 UTC
ok, closing. Please reopen if still relevant/you want to work on it.

Comment 15 Michal Skrivanek 2020-04-01 14:50:09 UTC
ok, closing. Please reopen if still relevant/you want to work on it.