Add -bypass-auth-except-for to protect only paths matched by a regex

[mrogers950]

This option takes priority over --skip-auth-regex if set. When unset, all paths
are protected by default.
Fixes #42
@smarterclayton @simo5 @enj please review :)

[mrogers950]

I noticed login does not work if you don't include the proxy prefix (for {prefix}/start, etc.) in the auth-regex. So we should always include a match for paths under the prefix.
Also, I don't see the value in allowing both options to be set so I plan on making that a configuration error.

[simo5]

I think we need tests ?

[simo5]

I feel like we need to make more clear that when this isused the other paths will be without any authentication, so it "fails open". IE use it only if you really know what you are doing, Maybe we need a scarier name too ?

[mrogers950]

I changed it to error out if both options are set, and fixed the problem with the proxy-prefix paths I mentioned above. I updated the option help as well, hopefully that is clearer.

I think we need tests ?

right, I think these would be most appropriate for the e2e testing.

[smarterclayton]

Why make it exclusive? I'd expect -auth-regex to be a positive check (only auth check these things) and -skip-auth-regex to punch holes in -auth-regex.

…

On Thu, Jan 4, 2018 at 5:48 PM, Matt Rogers ***@***.***> wrote: I changed it to error out if both options are set, and fixed the problem with the proxy-prefix paths I mentioned above. I updated the option help as well, hopefully that is clearer. I think we need tests ? right, I think these would be most appropriate for the e2e testing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pxw-or4LqnH1tJXAlGUnS1wP29Ziks5tHVUugaJpZM4RTubF> .

[mrogers950]

Why make it exclusive?

I'd expect -auth-regex to be a positive check (only auth check these
things) and -skip-auth-regex to punch holes in -auth-regex.

@smarterclayton I thought it would be best to make them exclusive so there is less chance for a user to configure an insecure endpoint by mistake.

[simo5]

I think we really need a scarier name that makes users think twice. They need to realize from the get-go that this command is disabling authentication on all but the listed paths, but still letting everything through on the other paths.

I do not care much for combining the old and new options, ok for me if they stay separate.

Also ok to reconsider using a different regex engine in the original option and drop this new option.

[mrogers950]

I think we really need a scarier name that makes users think twice. They need to realize from the get-go that this command is disabling authentication on all but the listed paths, but still letting everything through on the other paths.

How about renaming (or aliasing for now) skip-auth-regex to something like:
--bypass-auth-for=
and add this option as
--bypass-auth-except-for=
This makes the combination of the two less confusing to me.

Also ok to reconsider using a different regex engine in the original option and drop this new option.

There is https://github.com/dlclark/regexp2 which supports negative lookaheads, but not everything that the standard regex does so switching might break some users.

[enj]

There is https://github.com/dlclark/regexp2 which supports negative lookaheads, but not everything that the standard regex does so switching might break some users.

You could define another mutually exclusive flag--skip-auth-regex-v2-super-awesome that a user can opt into for the more advanced regex bits.

[enj]

Or you could add a flag --use-advanced-regex which swaps out what regex engine is being used.

[mrogers950]

@simo5 @enj I updated the option as -bypass-auth-except-for and added a similarly named alias for skip-auth-regex. I looked into the approach of swapping out the regex engine, but that was more complex especially with the need to keep the proxy-prefix paths protected when excluded by the regex.

[simo5]

/lgtm

[simo5]

@smarterclayton we kept the two options incompatible, and not compuinding them, you expressed surprise at that. If you feel strongly please shout. Otherwise we are going to merge this soon.

[simo5]

Ok I guess we'll have a followup PR is @smarterclayton feels really strongly about this

[smarterclayton]

I'm ok with them being incompatible for now.