Skip to content
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

Suppress AbstractProfile::CA_CLASH_ADDED for a new CA with the same public key #222

Merged
merged 1 commit into from May 14, 2021

Conversation

zmousm
Copy link
Contributor

@zmousm zmousm commented May 12, 2021

Fix #221

Skip AbstractProfile::CA_CLASH_ADDED when the new CA has the same name with an existing CA, but also an identical public key.

@restena-sw
Copy link
Contributor

Attempting to identify a root vs. cross-signed equivalent CA by its name is not sufficient.

The whole alerting feature is about a (hyporthetical) attacker taking over the account of an inst admin, and then replacing the root CA with one that has an identical name but is under control of the attacker. It is easily possible to synthesize a CA that has many of the same properties as the expected original.

So, merely comparing the name is something that can easily be catered for by an attacker.

The only property that cannot be synthesized by the attacker is the public/private keypair. And here a modified PR would make sense: additionally to the name, also compare if the public key of the intermediate version matches the public key of the root variant. Only if they are both identical, then we can be sure it is the "same" CA (in the sense that the person/organisation controlling the old cert's private key is also the organisation controlling the new cert's private key).

Please modify your PR to compare public keys, too. In the example you gave in issue #221, this would be the key:

Subject Public Key Info:
Public Key Algorithm: rsaEncryption
RSA Public-Key: (4096 bit)
Modulus:
00:c2:f8:a9:3f:1b:89:fc:3c:3c:04:5d:3d:90:36:
b0:91:3a:79:3c:66:5a:ef:6d:39:01:49:1a:b4:b7:
cf:7f:4d:23:53:b7:90:00:e3:13:2a:28:a6:31:f1:
91:00:e3:28:ec:ae:21:41:ce:1f:da:fd:7d:12:5b:
01:83:0f:b9:b0:5f:99:e1:f2:12:83:80:4d:06:3e:
df:ac:af:e7:a1:88:6b:31:af:f0:8b:d0:18:33:b8:
db:45:6a:34:f4:02:80:24:28:0a:02:15:95:5e:76:
2a:0d:99:3a:14:5b:f6:cb:cb:53:bc:13:4d:01:88:
37:94:25:1b:42:bc:22:d8:8e:a3:96:5e:3a:d9:32:
db:3e:e8:f0:10:65:ed:74:e1:2f:a7:7c:af:27:34:
bb:29:7d:9b:b6:cf:09:c8:e5:d3:0a:fc:88:65:65:
74:0a:dc:73:1c:5c:cd:40:b1:1c:d4:b6:84:8c:4c:
50:cf:68:8e:a8:59:ae:c2:27:4e:82:a2:35:dd:14:
f4:1f:ff:b2:77:d5:87:2f:aa:6e:7d:24:27:e7:c6:
cb:26:e6:e5:fe:67:07:63:d8:45:0d:dd:3a:59:65:
39:58:7a:92:99:72:3d:9c:84:5e:88:21:b8:d5:f4:
2c:fc:d9:70:52:4f:78:b8:bd:3c:2b:8b:95:98:f5:
b3:d1:68:cf:20:14:7e:4c:5c:5f:e7:8b:e5:f5:35:
81:19:37:d7:11:08:b7:66:be:d3:4a:ce:83:57:00:
3a:c3:81:f8:17:cb:92:36:5d:d1:a3:d8:75:1b:e1:
8b:27:ea:7a:48:41:fd:45:19:06:ad:27:99:4e:c1:
70:47:dd:b5:9f:81:53:12:e5:b1:8c:48:5d:31:43:
17:e3:8c:c6:7a:63:96:4b:29:30:4e:84:4e:62:19:
5e:3c:ce:97:90:a5:7f:01:eb:9d:e0:f8:8b:89:dd:
25:98:3d:92:b6:7e:ef:d9:f1:51:51:7d:2d:26:c8:
69:59:61:e0:ac:6a:b8:2a:36:11:04:7a:50:bd:32:
84:be:2f:dc:72:d5:d7:1d:16:47:e4:47:66:20:3f:
f4:96:c5:af:8e:01:7a:a5:0f:7a:64:f5:0d:18:87:
d9:ae:88:d5:fa:84:c1:3a:c0:69:28:2d:f2:0d:68:
51:aa:e3:a5:77:c6:a4:90:0e:a1:37:8b:31:23:47:
c1:09:08:eb:6e:f7:78:9b:d7:82:fc:84:20:99:49:
19:b6:12:46:b1:fb:45:55:16:a9:a3:65:ac:9c:07:
0f:ea:6b:dc:1f:2e:06:72:ec:86:88:12:e4:2d:db:
5f:05:2f:e4:f0:03:d3:26:33:e7:80:c2:cd:42:a1:
17:34:0b

@zmousm
Copy link
Contributor Author

zmousm commented May 12, 2021

Thanks @restena-sw

I understand I mixed up the root flag anyway (it should be old=0, new=1).

Furthermore, per your comment, I now think I should only check for a matching public key in order to skip this warning, regardless the type of CA. Your thoughts?

As I could not find any established facility to inspect the public key or modulus, I added fetching the string representation, which I then just compare, so as to not have to deal with different key types/algorithms. Please advise if this is an acceptable approach.

PS: I will subsequently squash the additional commit(s) and update messages and description.

@restena-sw
Copy link
Contributor

This looks very good! Happy to merge this and backport to release_2_0 branch after squashing.

And yes, it doesn't matter if the status changed from intermediate to root either. One could also imagine re-issuing an intermediate as intermediate; or even re-issuing a root with some differing properties. It's still the "same" CA then, and there is no need for the overly alerting warning.

The public key (and by that, private key) being identical is the only thing that really counts.

@zmousm zmousm changed the title Suppress AbstractProfile::CA_CLASH_ADDED for cross-signed intermediate replaced by root with same name Suppress AbstractProfile::CA_CLASH_ADDED for a new CA with the same public key May 13, 2021
@zmousm
Copy link
Contributor Author

zmousm commented May 13, 2021

Thanks @restena-sw, all done

@restena-sw restena-sw merged commit f106de4 into GEANT:master May 14, 2021
restena-sw added a commit that referenced this pull request May 14, 2021
Suppress AbstractProfile::CA_CLASH_ADDED for a new CA with the same public key
@zmousm zmousm deleted the tweak_ca_clash branch May 14, 2021 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants