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

Proposal: add trust-all-certs option. #727

Conversation

a-andreyev
Copy link

Hello! Thank you for the project! :)

The proposal is to add an option to trust any incoming cert, what do you think?

It could be convenient for the connman and NetworkManager plugin developers not to crash the process if the incoming cert hash is not received on the client side yet.

The better approach is probably to implement the full-fledged async control interface to accept the incoming certificate digest, but I'm not sure what IPC protocol to choose then (sockets/dbus/etc).

Additional context is the connman plugin development for the mer/sailfish/aurora mobile linux distributions: https://git.sailfishos.org/mer-core/openfortivpn/merge_requests/2

Feel free to reject/criticize, would be happy to get feedback.

Gateway certificate won't be rejected after the failed check
if `trust-all-certs` is set.

Signed-off-by: Alexey Andreev <[email protected]>
Signed-off-by: Andrey Okoshkin <[email protected]>
@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented May 29, 2020

I think it's a bad idea. In any case it has been already discussed in #468 for example.

That said I'm not sure I fully understand the use case. For example what do you mean by "the incoming cert hash is not received on the client side yet"? How does the NetworkManager-fortivpnssl process crash?

@a-andreyev
Copy link
Author

Thank you for the fast response and the previous discussion link!

You're probably right. I should take a look here #468 (comment) and #468 (comment) and probably update my approach or close as a duplicate.

My initial motivation was that there's a logic in openfortivpn to get incoming sha256 cert digest. I thought it would be convenient to have an option to somehow receive it. The received digest could be used on the network manager / GUI side then. But I'm not sure is it a good security/convenience balance.

My bad in the wording also about the NM crash (sorry, not a native speaker). Not crash, but expected behavior, when the incoming cert digest is shown in the logs and the process finished.

@DimitriPapadopoulos
Copy link
Collaborator

OK, I see:

  • Indeed openfortivpn receives the public certificate of the VPN gateway - and verifies it against the CA certificates it has in store.
  • Indeed openfortivpn will exit if the VPN gateway certificate cannot be verified. This is how TLS connections are supposed to operate.
  • While NetworkManager-fortisslvpn could get the certificate from openfortivpn, it is (currently) not really the purpose of openfortivpn to interoperate very broadly with other programs such as NetworkManager-fortisslvpn. Instead I think it makes sense for NetworkManager-fortisslvpn to download the certificate directly from the VPN gateway on the first attempt to connect to a VPN gatewey, before calling openfortivpn. It shouldn't be too complicated, see SSL/TLS Client. So on the first connexion to a VPN gatweay, NetworkManager-fortisslvpn could verify the certificate itself and, if verification fails, ask the user if it's OK to trust the VPN gateway despite the broken certificate.
  • Finally I'm all for allowing better communication between openfortivpn and other software such as NetworkManager-fortisslvpn using some IPC mechanism - see Move routing and name resolution changes into an external script? #678 (comment). Which mechanism though? I'm open to suggestions.

@DimitriPapadopoulos
Copy link
Collaborator

@a-andreyev I'm thinking of closing this pull request. Instead I suggest we continue the discussion about integration of openfortivpn and GUI in existing open issues or in a new issue. Is that OK with you?

@a-andreyev
Copy link
Author

Sorry for a late reply! Yes, I guess it makes sense to continue the discussion in the existing open issues.

@a-andreyev a-andreyev closed this Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants