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

[PATCH] make client-side authentication methods optional #513

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jkroepke
Copy link

@jkroepke jkroepke commented Mar 5, 2024

Ref #501

I would like to start an discussion about this patch. If the discussion has a positive outcome, I will sent this patch to the via mail.

@schwabe
Copy link
Contributor

schwabe commented Mar 6, 2024

I am not sure this is a good way forward. I don't think truly having no client side authentication is not something that I can see has any value for a client connection. I think it would be better to instead introduce something like expect-pending-auth that would tell the client that needs to fail the connection if no pending auth request is received from the server (unless the client has an auth-token/auth-token-user).

Users are often doing incredibly stupid things and allowing to configure a VPN connection without authentication is something that will backfire and then blaming us for their mistakes. And it is a very dangerous feature without safeguards like I describe in the paragraph before.

@selvanair
Copy link
Contributor

Does enforcing that some form of authentication is required in client config really provide any safeguard?

Its the server that authenticates and if there is verify-client-cert none and none of auth-user-pass-verify variants or other authentication mechanisms in the server config, none of this at client side is going to help. Its the server that must fail clients that do not have the required authentication settings.

I'm not convinced that the current client-side restriction serve any purpose at all.

@jkroepke
Copy link
Author

jkroepke commented Mar 6, 2024

Users are often doing incredibly stupid things and allowing to configure a VPN connection without authentication is something that will backfire and then blaming us for their mistakes.

By default, OpenVPN requires server-side authentication. Users have to opt-out with verify-client-cert none on the server to run auth-less. A warning is also raised on the logs, if verify-client-cert none is defined.

A clients-side restriction still doesn't protect against an server-sided insecure configuration.

Incredibly stupid users can still do incredibly stupid things. For example by just define

<auth-user-pass>
username
password
</auth-user-pass>

on the configuration, which is the current workaround for this issue.

@schwabe
Copy link
Contributor

schwabe commented Mar 6, 2024

Yes, you can always find a way too shot them into the foot. And I think being able to disable client side without any warning or extra option is dangerous cause it could lead to accidentially insecure configuration. I think this needs to be explicitly requested to avoid these situations.

@jkroepke
Copy link
Author

jkroepke commented Mar 7, 2024

Would an additional config directive "no-auth" sufficient?

The config directive would only skip the if condition, which is remove here. No additional logic.

@selvanair
Copy link
Contributor

Would an additional config directive "no-auth" sufficient?

If we have to go this route, --external-auth may be a better choice for the option name. Just add a value based on it to the variable sum and the if clause will get bypassed. The option parsing could be kept simple by allowing it to be specified irrespective of any other option as the only place it will get interpreted is for this check.
That said, we are just piling on to a check that is in the wrong place to start with (client instead of server) -- but it seems only I think that way.

@cron2
Copy link
Contributor

cron2 commented Mar 8, 2024

That said, we are just piling on to a check that is in the wrong place to start with (client instead of server) -- but it seems only I think that way.

I actually agree with Selva here - whether or not there is authentication is a server side decision.

That said, I think I do understand where Arne is coming from - users put incredibly stupid stuff into their configs, then things fail, and we get bad press out of it. So I'd go with making it explicit, as Selva proposed, adding an --external-auth or --expect-external-auth switch, with a warning label attached.

(And then Arne gets the prize for making us introduce yet another option)

@jkroepke
Copy link
Author

jkroepke commented Mar 8, 2024

Since, it's my first contribution and I would like to reduce the review cycle and make you more happy.

Where I have to add the new option?

  • config parser
  • config validator (changing the sum calculation)
  • --help ?
  • man page

somewhere else?

@selvanair
Copy link
Contributor

Where I have to add the new option?

config parser
config validator (changing the sum calculation)
--help ?
man page

That seems to cover it all. Also, Gert wants a warning, which could be possibly added like this:

Leave the definition of "sum" as is. If it's zero, and no indication of external auth, enter the if clause and exit. Else, if sum is zero, log a warning that this configuration assumes that the server will authenticate the user via external means such as webauth. Something like that.

@cron2
Copy link
Contributor

cron2 commented Mar 8, 2024

I do not need a warning in the actual code. Having a warning in the manpage is perfectly fine for me.

But Selva's suggestion would also work, and I think would make Arne more happy ;-)

@jkroepke jkroepke marked this pull request as draft March 16, 2024 13:55
@jkroepke jkroepke force-pushed the remove-user-credentials branch 2 times, most recently from 4c70e70 to 53e363a Compare March 16, 2024 14:01
@jkroepke jkroepke marked this pull request as ready for review March 16, 2024 14:08
@jkroepke
Copy link
Author

Tested locally on a linux system. It works fine. Tested with external-auth and without. If external-auth is omit, the current behavior applies. With external-auth, I'm able to authenticate via WEBAUTH, which configure additional credentials.

doc/man-sections/client-options.rst Outdated Show resolved Hide resolved
doc/man-sections/client-options.rst Outdated Show resolved Hide resolved
src/openvpn/options.c Outdated Show resolved Hide resolved
@jkroepke jkroepke force-pushed the remove-user-credentials branch from 53e363a to ea545a0 Compare March 17, 2024 09:03
@jkroepke
Copy link
Author

Thanks for the review, I have adjust the text. I also really recommend the suggest feature to request changes more precisely.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request

Copy link
Contributor

@selvanair selvanair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@schwabe
Copy link
Contributor

schwabe commented Mar 17, 2024

I would rather have a different name than external-auth as you still have auth-pending methods that can be internal to OpenVPN like 2FA or similar. I would rather spend the extra effort of implementing a flag like expect-pending-auth and check that this has happend at the connected state. Or at least have a better name than external-auth

@jkroepke
Copy link
Author

What about an flag named no-client-credential instead spending the extra effort?

Instead expecting something from server, declare that I don't have client credentials?

@jkroepke
Copy link
Author

I pushed a new state based on my last proposal

@jkroepke
Copy link
Author

jkroepke commented Apr 5, 2024

I this fine and ready to review through mailing list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants