Better compatibility with Okta as IDP - Custom scopes and escaped groups in tables

[yonatan-sevenai]

Summary:

This pull request introduces enhancements to the login command and policy file parsing functionalities. It includes support for custom scopes in the provider argument handling in the login command and adds new utility functions allowing groups with spaces in their names (such as "group name" to be valid in /etc/opk/providers)

While this was designed with Okta as the test-case for IDPs, nothing in this code is specific to Okta.
Two new capabilities are supported

1. ./opkssh add user "oidc:groups:group with space" https://some.sso.domain - This will add the group "group with space" and allow it to verify in oidc:groups verifications.
2. ./opkssh login --provider=https://subdomain.okta.com,client_id,,groups - This will login to the application in Okta adding the "groups" scopes to have okta include group information

Detailed Description:

Changes in commands/login.go:

• Provider Argument Parsing:
  • Modified the validation logic for the provider argument to support an additional format: <issuer>,<client_id>,<client_secret>,<additional_scopes>.
  • Updated the error message to reflect the new expected format.
  • Introduced logic to handle additional scopes by appending them to the opts.Scopes if provided for custom providers.

Changes in policy/files/table.go:

• New Utility Functions:
  • Added fieldsEscaped: A function to split strings on whitespace boundaries while preserving escaped spaces and backslashes.
  • Added writeEscaped: A function to join fields into a single string, escaping spaces and backslashes to ensure correct parsing.
• Table Parsing and String Conversion:
  • Updated NewTable to use fieldsEscaped for parsing rows, allowing for more flexible handling of spaces and backslashes in policy files.
  • Updated ToString to use writeEscaped for converting rows back to strings, ensuring that spaces and backslashes are correctly escaped.

Impact:

• Login Command: Users can now specify additional scopes in the provider argument, supporting OIDC providers who don't include group information by default.
• Policy Files: Improved handling of spaces and backslashes in policy files, supporting groups with spaces in their names.

[EthanHeilman]

Thanks for this, I won't get a chance to review until next week

[hendrik1120]

Just stumbled upon this while wondering about the scopes requested for the custom provider.
Wouldn't it be better to allow users to remove scopes as well, instead of just adding them?

Currently, opkssh requests the profile scope for every custom provider, even though it is not used. This is still an artifact from the reliance on the Google defaults.
Maybe only append new scopes based upon the openid scope since that is a strict requirement?

Please don't make any changes to this PR based on my comment until @EthanHeilman has had the time to look at it as well.

[yonatan-sevenai]

I think the ideal way would be to refactor how configuration of providers is done and externalize this to a config file.
In that file one could provide the URL, clientID, secret, icon, short-name, etc...

This way, the login command can take a shortcut to an existing provider, and the web-ui can be rendered based on your existing providers.
I wrote this PR to quickly solve some challenges we've had integrating OPKSSH (scopes was missing from Okta requests) and lack of support for group names with spaces in them...

Not sure I'll have time to whip the above together, so this is more in lines of wishful thinking at this point.

[yonatan-sevenai]

As a stop gap, we might say that the only scope being requested is "openid" and, when scopes are provided, the user must specify if they want profile, email, groups, etc...

[yonatan-sevenai]

I'll try to join.

[EthanHeilman]

Maybe only append new scopes based upon the openid scope since that is a strict requirement?

I like the intention behind this of protecting people from themselves, in this case I don't think we should append.

If scopes are specified, we should only use those scopes even tho someone might not specify the openid scope and then discover it doesn't work. It is better for them to realize they need the openid scope, then to hide this fact from them. Additionally there is always going to be some weird provider that uses weird scopes and breaks if you specify openid instead of open_id

As far as I can tell I believe specifying scopes was added in another separate PR:

// ProviderConfig is the representation of the provider config:
// {alias},{provider_url},{client_id},{client_secret},{scopes}
// client secret is optional, as well as scopes, if not provided, the default for secret is an empty string, for scopes is "openid profile email"
type ProviderConfig struct {
	Alias        string
	Issuer       string
	ClientID     string
	ClientSecret string
	Scopes       []string
}

https://github.com/openpubkey/opkssh/blame/97b81e96004d3a6931207736ba151e8374ba4d35/commands/login.go#L470-L479

[EthanHeilman]

Fix the merge conflict with the prior scopes change and then add a unit test. Other than that this looks good to merge

[yonatan-sevenai]

I removed the scoped portion of my PR as it was already covered in https://github.com/openpubkey/opkssh/blame/97b81e96004d3a6931207736ba151e8374ba4d35/commands/login.go#L470-L479.
Leaving the table escaping logic to support OIDC groups with spaces in the name

[yonatan-sevenai]

@EthanHeilman - Can you re-review and merge? This will allow support for group names with spaces in the tables.

[EthanHeilman]

Thanks for this PR. It looks really good. I appreciate the work you put in and I'm eager to merge this.

I want to make sure this won't break any existing files:

• Add an end to end test from an policy file with escaped spaces to a policy enforcer check.
• Add a provider file unit test

I am slightly worried that someone that has a \ in their provider config this change would make it no longer match (see unit test I added below).

For instance someone had a provider line like
https:://issuer.example.com asdfsdfsd\bdfsdfsd 24h
which currently would match for the clientID asdfsdfsd\bdfsdfsd but after this change would no longer match. This seems unlikely but I want to extra cautious about anything that could lock someone out.

I edited the PR description since it was out of date.

[markafarrell]

Is there a reason we are rolling our own here instead of using something like shlex?

[EthanHeilman]

@markafarrell That is an excellent point. @yonatan-sevenai is there any downside to using @shlex here? It gets us quoting as well which is another requested feature

[yonatan-sevenai]

@EthanHeilman, @markafarrell - Thanks for this note.

I think we should stick with the current implementation for the following reasons

1. The shlex library doesn't support writing to shell escaped, meaning we'll need to roll out our own writing and hope it's compatible with shlex on all use-cases. Incompatibility will break users table files (potentially in subtle ways). We can try to look for other implementations, but for them as well, incompatibility might be an issue
2. (Aesthetic) I personally find entries in config files with quotes in them and other shell escapes.

If anything, as part of the YAML user config, we can start moving all configurations to YAMLs which will be more readable than current table based approach.
I suggest we merge this as is, and start thinking of migration of the entire configs to YAML.

[EthanHeilman]

@yonatan-sevenai I want to think through this case by case, please excuse my pedantry:

Where unescaped spaces might be a problem

Specify a provider on the client side

• Unescaped spaces are not an issue in the yaml client config file,
• In the provider argument you can just quote it and it works today opkssh login --provider="https://subdomain.okta.com,client_id,,groups,openid email profile"
• With environment variables you can also use quotes OPKSSH_PROVIDER="https://subdomain.okta.com,client_id,,groups,openid email profile"

Specifying a provider on the server side

In /etc/opk/providers you only need to specify the issuer, clientID and expiration. None of which have spaces.

Specifying policy

/etc/opk/auth_id is the main problem area.

{principal} {requirement} {issuer}

Neither the principal nor the issuer will contain spaces, so the only issue is the requirement part of the policy

root "oidc:groups:group with space" https://some.sso.domain

or

root oidc:groups:group\ with\ space https://some.sso.domain

Wouldn't
./opkssh add user "oidc:groups:group with space" https://some.sso.domain
adds the line:
user "oidc:groups:group with space" https://some.sso.domain
and
./opkssh add user oidc:groups:group\ with\ space https://some.sso.domain
adds the line:
user oidc:groups:group\ with\ space https://some.sso.domain
be the least complex and what the user expects?

@markafarrell
shlex gets of this out of the box right?

I suggest we merge this as is, and start thinking of migration of the entire configs to YAML.

The PR needs to address the disappearing escapes before this can be merged.

That said, I'm leaning toward using shlex here. We could just quote all policy requirements that have a space.

[markafarrell]

To assist with the discussion this is what it would look like using :

• shlex Use shlex for parsing policy::Table #157
• shellquoteUse shellquote for parsing policy::Table #158

[yonatan-sevenai]

@EthanHeilman - Regarding your comment - the CLI side is okay as you listed. In fact, we don't need to do anything as the shell will correctly escape shell for us.

The problem is purely for the on-disk table format. The current implementation breaks with the table on-disk format as it uses spaces to separate fields.
As long as opkssh uses tables for /etc/opk/auth_id we need to implement a solution.

Another thing to remember is that we need to both read and write to tables in a consistent way.

Therefore, using shlex, for example, doesn't solve us the write problem, so we can have one implementation write to the table, and shlex for reading, but this might introduce inconsistencies. For example, @EthanHeilman suggested just quoting every word before writing, but this will break if an input string has a " (quote) character in it. While not likely legal for groups names, I'm sure we can find some OAuth provider that does allow it...

As for shellquote, since it seems it properly supports both read and write (i.e. split and join), this seems to be a viable option, as long as we feel comfortable with backwards compatibility with existing /etc/opk/auth_id files and properly test where there are control characters in the input string

[EthanHeilman]

@yonatan-sevenai Your backwards compatibility concern is that someone might have a line like:

user oidc:groups:abc\~mo https://some.sso.domain

which we would unescape to oidc:groups:abc~mo but the user intend was to actually match on oidc:groups:abc\~mo?

[markafarrell]

Is it worth raise a PR to document the current behaviour in table_test.go so we understand exactly the behaviour change that we are introducing?

[yonatan-sevenai]

@EthanHeilman - The example of an escape or quote within a group name is the scenario I had in mind.
Frankly, it sounds very esoteric, but such a case could mean someone losing access to their server.

There's a work-around, in introducing a version line to the table format, and reverting to the existing behavior if no such version line appears consider starting each table with the line OPKSSHTBLV1.

[yonatan-sevenai]

We're moving to #158 as the base implementation.
Closing this for now.