ipsec: T8136: IPSEC PPK support #4930
Conversation
|
👍 |
giga1699
force-pushed
the
T8136
branch
3 times, most recently
from
cf026ea to
0f19483
Compare
sarthurdev
requested review from
c-po,
dmbaturin,
natali-rs1985 and
sever-sever
sarthurdev
left a comment
sarthurdev
left a comment
There was a problem hiding this comment.
This is looking good, can you extend ipsec smoketests for the new use cases?
|
I was going to make an attempt to write a smoke test. It’ll be my first run at writing a smoketest. I have already tested against known working strongSwan configs. I’ll take those tests to put into smoketest and update. I’m also planning to put together the documentation PR. Just haven’t gotten to that yet. I’ll update the initial PR comment to reflect these updates when complete, and take this out of draft. |
|
Added relevant smoketest, and completed documentation update. I think this should be ready to go. |
sarthurdev
left a comment
sarthurdev
left a comment
There was a problem hiding this comment.
Thanks for the smoketest.
Just needs couple extra steps for config validation and XML deduplication.
|
Much appreciated for catching those items. It got me thinking about some additional error checking. I added in that PPK IDs need to be unique, so it now checks that all IDs are unique. Since I had to gather all the IDs for that check, the verify() also confirms that a PPK ID used in a connection is actually defined in the authentication config. It might be useful to replicate that check for PSKs. I'm happy to open a new task and take care of that if it would be helpful. Branch was also rebased to current. |
src/conf_mode/vpn_ipsec.py
Outdated
| for ppkID in ppk_config['id']: | ||
| if ppkID in ppk_ids: | ||
| raise ConfigError(f'Authentication PPK "{ppk}" has duplicate ID "{ppkID}" from another PPK. IDs should be unique.') |
There was a problem hiding this comment.
In my testing if you use the same ID for multiple PPKs, it seems like strongSwan will only use the last loaded PPK for that ID. That is why I put in a check. The ID needs not be an IP address, it is a string that is passed in the IKE phase 1 to let the other side know I'm using key XX. If the receiving side has a key with that same key ID, then it will use that secret to try and establish the SA.
|
Removed the check in the connection configurations to confirm that a PPK ID was defined in the authentication configuration. This allows an operator to define a wildcard match for the PPK ID, as referenced in the strongSwan example PPK configuration. |
sever-sever
left a comment
sever-sever
left a comment
There was a problem hiding this comment.
Add the ability to use PPK ID and secrets for IPsec connections
giga1699
force-pushed
the
T8136
branch
2 times, most recently
from
117394b to
a641609
Compare
alexandr-san4ez
left a comment
alexandr-san4ez
left a comment
There was a problem hiding this comment.
Please also fix linter errors for your changes.
alexandr-san4ez
left a comment
alexandr-san4ez
left a comment
There was a problem hiding this comment.
Post-quantum preshared key support well implemented and ready to merge.
vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_vpn_ipsec.py
...
test_site_to_site_nist_800_77_cnsa_1_with_ppk (__main__.TestVPNIPsec.test_site_to_site_nist_800_77_cnsa_1_with_ppk) ... ok
...
----------------------------------------------------------------------
Ran 20 tests in 101.409s
OK
sarthurdev
left a comment
sarthurdev
left a comment
There was a problem hiding this comment.
Requested changes addressed, code looks good, adds smoketest coverage. Locally verified CLI and resulting strongswan config/instance.
dmbaturin
left a comment
dmbaturin
left a comment
There was a problem hiding this comment.
The implementation seems sensible but I left a few suggestions.
dmbaturin
left a comment
dmbaturin
left a comment
There was a problem hiding this comment.
I applied fixes for my aesthetic objections. The logic seems fine, I have reservations about the idea to show up-ppk as a distinct status but I don't think anything in the PR is problematic.
I wasn’t sure the best way to indicate PPK in use without modifying the table more. If that’s a better approach, I would be open to it. Putting parenthesis around ppk after up made it stray into the next column more, making readability more difficult. |
zdc
left a comment
zdc
left a comment
There was a problem hiding this comment.
Good work here!
Beyond the minor changes I noted in the code review, I’d like to clarify the use of childless. Is this mandatory for PPK? If so, it should be addressed in verify() and documented - currently, I don’t see it mentioned anywhere. If it’s not mandatory, I think it warrants a dedicated PR with a clear explanation.
P.S. I have a theory on why it might be mandatory, but either way, this should be covered in both the code and documentation.
| if 'state' in sa and s(sa['state']) == 'ESTABLISHED': | ||
| if 'ppk' in sa and s(sa['ppk']) == 'yes': | ||
| state = 'up-ppk' | ||
| else: | ||
| state = 'up' | ||
| else: | ||
| state = 'down' |
There was a problem hiding this comment.
I think it's better to introduce a new field with PPK presense. State variable values may be used in scripts and automations already, including customer-made.
There was a problem hiding this comment.
I think any change to the table output may affect scripts either way. Do you think adding a column at the end for PPK would be the best approach? Would that cause the text to extend too far to the right, and possibly wrap around on certain consoles?
| for ppk_id in ppk_config['id']: | ||
| if ppk_id in ppk_ids: | ||
| raise ConfigError( | ||
| f'Authentication PPK "{ppk}" has duplicate ID "{ppk_id}" from another PPK. IDs should be unique.' | ||
| ) | ||
| ppk_ids.append(ppk_id) |
There was a problem hiding this comment.
@giga1699 I've seen your comment about this.
As I know, for PSK, the logic is a bit more complicated, and it actually allows to have same ID in multiple entries. If strongSwan uses the same logic, this check may not be entirely correct or complete.
Can you point to the relevant place in the strongSwan documentation or the code where this is explained for PPK?
There was a problem hiding this comment.
A PPK ID is identifying a specific secret. A secret may share multiple IDs, but two separate secrets should not have a common ID. If I define secret "abc" with ID of "ID-1", then later define secret "def" with the same ID of "ID-1", only one of those two secrets will actually be tied with the ID "ID-1".
During IKE_AUTH, the PPK_ID is sent. This ID should be tied to a single shared secret between those devices. If multiple secrets were defined for the same ID, I'm not sure how any device would be able to determine which key to actually use. The PPK secret itself is used as part of the generation of the shared secret for the CHILD SA encryption, rather than solely as a way to identify the remote end like a PSK is used for. If both sides don't have the same shared secret negotiated, it wouldn't be able to pass traffic.
From RFC8784, section 1, paragraph 3
The general idea is that we add an additional secret that is shared between the initiator and the responder; this secret is in addition to the authentication method that is already provided within IKEv2. We stir this secret into the SK_d value, which is used to generate the key material (KEYMAT) for the Child Security Associations (SAs) and the SKEYSEED for the IKE SAs created as a result of the initial IKE SA rekey. This secret provides quantum resistance to the IPsec SAs and any subsequent IKE SAs. We also stir the secret into the SK_pi and SK_pr values; this allows both sides to detect a secret mismatch cleanly.
This check is to prevent an operator from accidentally defining two separate secrets that share the same ID. It is not preventing multiple IDs from being tied to the same secret.
This is perfectly valid where multiple IDs are tied to the same secret.
[edit vpn ipsec authentication]
vyos@vyos# show | commands
set ppk test id 'asdf'
set ppk test id 'fdsa'
set ppk test secret 'asdlgsgsakfhafuh4h83hf73hf83hd8'
This will generate properly in the swanctl.conf file
secrets {
ppk-test { # ID's from auth ppk <tag> id xxx id-0ed09c14-8073-4590-b722-dd09dce508bb = "asdf" id-0376dadf-59b2-4470-90c1-2aeb513175cc = "fdsa" secret = "asdlgsgsakfhafuh4h83hf73hf83hd8" }}
What would become an issue is if the above configuration was commited, but an operator tried to do the following, trying to set a different secret for the ID 'asdf'.
set vpn ipsec authentication ppk test-2 id 'asdf'
set vpn ipsec authentication ppk test-2 secret 'this should not be valid, or it will cause unknown behavior'
I hope this explains my thought process on this check. Maybe I'm not fully understanding where you're coming from, so please let me know if we're not on the same page.
@zdc Childless is not a requirement for PPK. It is recommend in NIST 800-77 to use childless with PPK to hep protect some information in the initial IKE exchange, but there is no mandate. Relevant excerpt from NIST 800-77:
We can certainly split the childless segment into a separate PR if you would prefer. RFC6023 lists multiple other use cases for childless.
I will work on the other changes today. |
This sounds like something that at least deserves a warning if PPK is configured without it. That was one of my concerns - if a user sets up PPK but doesn’t get full protection, it could be misleading and risky. |
I will work on a check that if PPK is enabled, but childless is not, it will put a warning that says childless is recommended for use with PPK. |
|
I think I have the minor revisions all complete. Just looking for some guidance on the best way to display PPK being in use, and to make sure everyone is on board with the check for unique PPK IDs. I went ahead and rebased as well to clean everything up. @zdc and/or @dmbaturin I'm very open to thoughts on the best way to show PPK being active. |
|
Testing from VyOS with only a single PPK set strongSwan 6.0.4 with two secrets set for the same PPK ID. Notice the correct one is set first, then the incorrect one is set second. Logs from strongSwan NOTICE
Change VyOS config Resulting strongSwan logs from Debian server |
|
CI integration 👍 passed! Details
|
6 participants
Change summary
Adds support for IKEv2 post-quantum preshared keys
Types of changes
Related Task(s)
https://vyos.dev/T8136
Related PR(s)
vyos/vyos-documentation#1733
How to test / Smoketest result
Checklist: