Check for elevated permissions with automatic login #482

gord5500 · 2024-09-24T12:50:54Z

Why

The --experimental-bypass-browser-security flag is only useful when running as admin.

I'm not sure though if we would prefer to not completely kill the process as is happening here or just skip over the automatic login area. I lean to killing it so people don't get use to having this flag included regularly

Ticket

VUL-453

Co-authored-by: Adipa Wijayathilaka <[email protected]>

gord5500 · 2024-09-24T13:39:26Z

src/D2L.Bmx/Program.cs

@@ -21,8 +23,28 @@
 // allow no-sandbox argument for chromium to for passwordless auth with elevated permissions
 var bypassBrowserSecurityOption = new Option<bool>(
 	name: "--experimental-bypass-browser-security",
+	getDefaultValue: () => false,


The validator wasn't getting called without this if the flag wasn't included

gord5500 · 2024-09-24T13:39:47Z

src/D2L.Bmx/Program.cs

@@ -282,3 +304,8 @@
 	} )
 	.Build()
 	.InvokeAsync( args );
+
+internal static partial class NativeMethods {


Top Level Statement L here?

I'd rather we wrap this along with the IsWindows, IsLinux, IsMacOS logic all in a separate file & class.

There's a lot happening in Program.cs already

boarnoah · 2024-09-24T13:45:14Z

src/D2L.Bmx/Program.cs

+	[LibraryImport( "libc", EntryPoint = "geteuid" )]
+	public static partial uint GetPosixEuid();


Do you know if this works on Alpine with musl, AFAICS the musl implementation of libc, is called libc as far as imports in C etc... are concerned.

So this might work?

our existing libc calls work in alpine

boarnoah · 2024-09-24T13:47:02Z

src/D2L.Bmx/Program.cs

+			""";
+	} else if( !isElevated && bypass ) {
+		result.ErrorMessage
+			= "BMX is not running with elevated permissions, so --experimental-bypass-browser-security must be set to false";


nit: Isn't it better to say, something along the lines of: don't provide -experimental-bypass-browser-security

I'd say this is more than a nit

Sure I'll reword

boarnoah · 2024-09-24T13:50:59Z

src/D2L.Bmx/Program.cs

+			Running BMX with elevated permissions is only allowed with the '--experimental-bypass-browser-security' flag.
+			Only include this if you aren't concerned with the security of the host for your Okta organization.


nit: I would really like it if we could articulate that:

Using BMX to do AWS stuff is perfectly fine while elevated

You only need this flag when you want to do the equivalent of bmx login as admin
a. Right call in most situations is to, run bmx login on your computer once a day without elevated privs
b. If you insist in running BMX with elevated privs as the first thing of the day, that is when you need the flag.

Not sure how to fit all that in one sentence :P

Okay so this checking for permission logic should go in the okta authenticator class then

There's also the consideration that... what if this flag is used with AWS credential process, and what if that may be triggered automatically sometimes elevated sometimes not?
This is a real concern for the LMS.

in other words, I'm not too sure that we should always fail BMX when this flag is present but the user isn't elevated

Yup I'll just move it to the okta authenticator and divert to password if it's not provided and they are admin

Okay I moved it to the authenticator where if bmx is running as admin and no flag provided, we fall back to password. If not admin but flag provided, we set the flag to false so '--no-sandbox' doesn't get included

I think that should also address the issue with wording you had Adipa since it's only on attempting to login now

boarnoah · 2024-09-24T13:56:14Z

I lean to killing it so people don't get use to having this flag included regularly

I would think we just silently not use the no-sandbox flag if we aren't escalated.

Set of things that lead us to have to use no-sandbox is kind of hard to convey to a user (you need to have a stale/close to stale okta session && you need to be trying to use bmx in that particular situation with escalated privs).

Regardless of what our users do poorly (IMO invoking tools that don't need escalation is such a thing) we should try to do the correct thing and try to not use no-sandbox whenever possible

CC @cfbao

EDIT: This also addresses Chenfeng's point about how credential process use of BMX means it could sometimes be escalated sometimes not

cfbao · 2024-09-24T14:01:07Z

@boarnoah
yeah I agree with Adipa here.
BMX should never use --no-sandbox when not elevated, but forcing not using the flag when not elevated onto our user might be detrimental to UX.

cfbao · 2024-09-24T17:09:31Z

src/D2L.Bmx/OktaAuthenticator.cs

+		bool hasElevatedPermissions = UserPrivileges.HasElevatedPermissions();
+		if( hasElevatedPermissions && !bypassBrowserSecurity ) {
+			consoleWriter.WriteWarning( $"""
+				BMX is being run with elevated privileges and is unable to automatically sign in to Okta.


Not in scope, but - some warnings messages are prefixed with "WARNING:" while others are not.
Would be nice to be consistent on this.
I lean towards no prefix, because we already use a different colour.

Hmm yea you're right. I started with including the prefix since they were in the bmx config provider class but have been inconsistent. I see we use WriteWarning in other places though without it. I think I'll remove the prefix for this class

cfbao · 2024-09-24T17:25:30Z

src/D2L.Bmx/OktaAuthenticator.cs

+				If you have to run BMX with elevated privileges, and aren't concerned with the security of {orgUrl.Host},
+				consider running the command again with the '--experimental-bypass-browser-security' flag.


Nit:
When this happens, user might just enter their Okta password and move on, so this message is just an "FYI for next time", and they won't want to run this same command again right now even if they have to run BMX elevated and aren't concerned with the security of the Okta site.

Maybe a better message should say "consider using this flag" instead of "consider running the command again".

Another thing that bugs me and it concerns other related warning messages too - if a user can't use DSSO at all, either because their Okta org doesn't support it, or their computer or network condition isn't right, then ideally BMX shouldn't show any DSSO related warning messages at all. I don't think the current implementation does this.
No rush to fix this right now, but I'd like us to do something about this before releasing.

I don't think the current implementation does this.
Yea if there was no DSSO implementation for an org, they would end up always seeing some warning messages about it

I think readding the passwordless flag could solved this where it is just defaulted to true.

more like... --disable-passwordless that's default false.
or another flag/config name.
We should probably still rephrase some messages though.
later

gord5500 and others added 30 commits September 17, 2024 09:16

okta: add dsso authentication

e9f18ac

add cancellation token to newpageasync

a16a9e9

tweak warning messages

6fc7baa

Update src/D2L.Bmx/OktaAuthenticator.cs

73d0cdd

var name tweak

c2f13bd

tweak warning message for non matching user

19fa544

mend

4f177b7

change reload signin page

aedb835

add passwordless option that defaults to false

c14de8e

mend

77a91c4

tweak warning message

1b8cfd2

set browser as headless based on bmx_debug env variable

1a8ba2e

readd path check

b526dbe

default browser to edge for windows

d502f1d

headless is always true regardless of bmx_debug

28b320a

make no-sandbox option scarier

7633b0f

abort if not on vpn

185b8ff

normalize okta org and check users route

d9fe294

remove passwordless flag

0080063

mend

ac51994

adjust parameter name for experimental

ff3f8e1

rename okta session function

551f6cf

redo org check

64f5efb

Update src/D2L.Bmx/Browser.cs

40171e4

Co-authored-by: Adipa Wijayathilaka <[email protected]>

nits

a2f903a

more nits

88f7517

readd user email strip check

91d52b6

mend

684f1bd

mend

81a3714

deal in uris instread of string for orgs

d48c7db

gord5500 commented Sep 24, 2024

View reviewed changes

gord5500 marked this pull request as ready for review September 24, 2024 13:40

gord5500 requested a review from a team as a code owner September 24, 2024 13:40

gord5500 requested review from cfbao, boarnoah, scowing, jharoutuniand2l and AnimalXing and removed request for a team September 24, 2024 13:40

boarnoah reviewed Sep 24, 2024

View reviewed changes

wording

afef184

gord5500 added 3 commits September 24, 2024 11:04

don't kill on bad setup

c956f2a

mend

9d717e8

mend

e47e88b

Base automatically changed from add_passwordless to main September 24, 2024 15:27

Merge branch 'main' into check_for_elevated_permission

7063367

cfbao previously approved these changes Sep 24, 2024

View reviewed changes

reword the warning message

521a53f

gord5500 dismissed cfbao’s stale review via 521a53f September 24, 2024 17:42

cfbao approved these changes Sep 24, 2024

View reviewed changes

gord5500 requested a review from boarnoah September 24, 2024 18:57

boarnoah approved these changes Sep 24, 2024

View reviewed changes

gord5500 merged commit 0401ee8 into main Sep 24, 2024
9 checks passed

gord5500 deleted the check_for_elevated_permission branch September 24, 2024 19:03

cfbao mentioned this pull request Oct 16, 2024

reword user-facing messages to say "passwordless" #485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for elevated permissions with automatic login #482

Check for elevated permissions with automatic login #482

gord5500 commented Sep 24, 2024

gord5500 Sep 24, 2024

cfbao Sep 24, 2024

gord5500 Sep 24, 2024

cfbao Sep 24, 2024

cfbao Sep 24, 2024

gord5500 Sep 24, 2024

boarnoah Sep 24, 2024

cfbao Sep 24, 2024

boarnoah Sep 24, 2024 •

edited

Loading

cfbao Sep 24, 2024

gord5500 Sep 24, 2024

boarnoah Sep 24, 2024

gord5500 Sep 24, 2024

cfbao Sep 24, 2024

cfbao Sep 24, 2024

gord5500 Sep 24, 2024

gord5500 Sep 24, 2024

gord5500 Sep 24, 2024

boarnoah commented Sep 24, 2024 •

edited

Loading

cfbao commented Sep 24, 2024

cfbao Sep 24, 2024

gord5500 Sep 24, 2024

cfbao Sep 24, 2024

gord5500 Sep 24, 2024

cfbao Sep 24, 2024

		[LibraryImport( "libc", EntryPoint = "geteuid" )]
		public static partial uint GetPosixEuid();

		Running BMX with elevated permissions is only allowed with the '--experimental-bypass-browser-security' flag.
		Only include this if you aren't concerned with the security of the host for your Okta organization.

		If you have to run BMX with elevated privileges, and aren't concerned with the security of {orgUrl.Host},
		consider running the command again with the '--experimental-bypass-browser-security' flag.

Check for elevated permissions with automatic login #482

Check for elevated permissions with automatic login #482

Conversation

gord5500 commented Sep 24, 2024

Why

Ticket

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boarnoah Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boarnoah commented Sep 24, 2024 • edited Loading

cfbao commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boarnoah Sep 24, 2024 •

edited

Loading

boarnoah commented Sep 24, 2024 •

edited

Loading