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

Expose AnonymousAccess service through Security OSS plugin. #87091

Merged
merged 15 commits into from
Jan 15, 2021

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Dec 31, 2020

In this PR I'm introducing a new Anonymous Access domain that spans both authentication and authorization pieces and will serve as a base for any future improvements related to the anonymous access.

There are four main changes in this PR:

  • X-Pack Security plugin creates a new AnonymousAccessService that can figure out if anonymous service account can successfully authenticate and what capabilities it has. It's registered with Security OSS plugin.
  • Security OSS plugin exposes /internal/security_oss/anonymous_access/capabilities Spaces aware endpoint that can be used to check what capabilities are available to anonymous service account using the registered AnonymousAccessService.
  • Security OSS plugin exposes the following anonymous access related functionality in its start contract:
anonymousAccess: {
  getAccessURLParameters: () => Promise<Record<string, string> | null>;
  getCapabilities: () => Promise<Capabilities>;
};
  • Security OSS plugin now hits /internal/security_oss/app_state endpoint in start to fetch the application state.

Prerequisite for: #86965 and #83650

@azasypkin azasypkin added release_note:enhancement feedback_needed Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Authentication Platform Security - Authentication Feature:Security/Authorization Platform Security - Authorization v7.12.0 labels Dec 31, 2020
@@ -51,8 +67,23 @@ export class SecurityOssPlugin
}

public start(core: CoreStart) {
const isAnonymousAccessEnabled = !!core.application.capabilities.security?.anonymousAccess;
Copy link
Member Author

Choose a reason for hiding this comment

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

note: My understanding and main assumption is that using endpoints and capabilities exposed by the X-Pack counterpart in the OSS version of the plugin is acceptable as long as it works fine with OSS distribution. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think capabilities are more OK than endpoints, but I don't know if we've codified this anywhere.

I don't love the idea of using x-pack endpoints in OSS, because that could lead to 404 errors, which is sloppy. I see you've guarded against that here, but I worry about the precedent this sets.

For the insecure cluster warning, we defined the endpoint in OSS and allowed x-pack security to augment the logic. This allows the endpoint to work in all distributions, while still allowing for distribution-specific behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't love the idea of using x-pack endpoints in OSS, because that could lead to 404 errors, which is sloppy. I see you've guarded against that here, but I worry about the precedent this sets.

For the insecure cluster warning, we defined the endpoint in OSS and allowed x-pack security to augment the logic. This allows the endpoint to work in all distributions, while still allowing for distribution-specific behavior.

Yeah, from the technical side I agree with your sentiment. It just feels to me that we're building all these abstractions only to pretend that there is no explicit dependency between OSS and X-Pack plugin parts when in reality the only purpose of the the X-Pack_Something_OSS plugins is to support their main plugins across distributions while being useless on its own. The insecure cluster warning is just a great exception that does its job even without X-Pack part though.

Let me see if I can find a better way to preserve a status quo here 😛

// We should use credentials of the anonymous service account instead of credentials of the
// current user to figure out if the specified Saved Object type can be accessed anonymously.
const anonymousAuthorizationHeader = this.createAnonymousAuthorizationHeader();
const fakeAnonymousRequest = KibanaRequest.from(({
Copy link
Member Author

Choose a reason for hiding this comment

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

question: do we have any better option to do what I do right now?

Copy link
Member

Choose a reason for hiding this comment

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

Not really 🙁. Core support for something better is being discussed in #39430, but for the time being this is the generally accepted short-term solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, thanks for confirming!

@@ -143,7 +166,7 @@ export class Plugin {
spaces,
}: PluginSetupDependencies
) {
const [config, legacyConfig] = await combineLatest([
this.configSubscription = combineLatest([
Copy link
Member Author

Choose a reason for hiding this comment

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

note: finally making our setup sync.

});

core.capabilities.registerProvider(() => ({
Copy link
Member Author

@azasypkin azasypkin Dec 31, 2020

Choose a reason for hiding this comment

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

note: I was hesitating about this one since it has nothing to do with the feature controls (FC) and UI capabilities are mainly used with FC right now. But if I understand the idea behind Core's/OSS UI capabilities then it's not necessarily related to the FC or Security in general, so I thought that anonymous access can be treated as a capability that may affect the way we render UI.

Am I just looking for excuse or you think it's a valid use case for UI capabilities as well? The main motivator is to avoid unnecessary call to the Kibana server on every page refresh just to know that anonymous access isn't enabled (yeah, I'm missing good old injected vars 🙈 )

@azasypkin
Copy link
Member Author

@legrego would you mind giving a preliminary feedback (tests are not ready yet) on the approach I picked in this PR, whenever you have time? I left a couple of questions for you.

Thanks!

@legrego legrego self-requested a review January 4, 2021 18:43
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Here are my preliminary thoughts - happy to discuss further! Thanks for putting this together ❤️

});

core.capabilities.registerProvider(() => ({
Copy link
Member

Choose a reason for hiding this comment

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

I'm also hesitant about this. UI Capabilities were designed to support FC, but that doesn't mean they can't be used for other purposes. That said, I'm not convinced this is the right tool for the job.

The main motivator is to avoid unnecessary call to the Kibana server on every page refresh just to know that anonymous access isn't enabled (yeah, I'm missing good old injected vars 🙈 )

I miss injected vars too! If the network call is the primary motivation, then perhaps we can tweak the security_oss plugin a bit. The OSS plugin makes a call to determine if the "insecure cluster warning" should be shown.

If we wanted to, we could introduce an /internal/security/setup (or similar) call, which can return information for both the "insecure cluster warning" and the state of anonymous access. That would keep the number of network calls the same as it is today, without changing the way that UI Capabilities are used. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we wanted to, we could introduce an /internal/security/setup (or similar) call, which can return information for both the "insecure cluster warning" and the state of anonymous access. That would keep the number of network calls the same as it is today, without changing the way that UI Capabilities are used. What do you think?

Yeah, I like that! I'll try to do that.

// We should use credentials of the anonymous service account instead of credentials of the
// current user to figure out if the specified Saved Object type can be accessed anonymously.
const anonymousAuthorizationHeader = this.createAnonymousAuthorizationHeader();
const fakeAnonymousRequest = KibanaRequest.from(({
Copy link
Member

Choose a reason for hiding this comment

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

Not really 🙁. Core support for something better is being discussed in #39430, but for the time being this is the generally accepted short-term solution.

);

if (hasAllRequested) {
this.logger.debug(
Copy link
Member

Choose a reason for hiding this comment

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

nit: While I'm generally a fan of logging all the things, these might be unnecessary. The return value here (which currently translates to the API route return value) isn't really transformed or fed into another server-side process.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was mostly using it to distinguish canAccess: false for the case when user cannot access something and when anonymous is not enabled. But yeah, it makes more sense to log it in the route instead (or wherever this API is used) 👍

return {
isAnonymousAccessEnabled: this.isAnonymousAccessEnabled(),
accessURLParameters:
anonymousProvider && !anonymousIsDefault
Copy link
Member

Choose a reason for hiding this comment

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

question is there harm in supplying the access url parameters if anonymous is the default provider? If an administrator changes the auth provider config, then they'd potentially invalidate any previously generated URLs if the anonymous provider suddenly isn't the default anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

question is there harm in supplying the access url parameters if anonymous is the default provider?

Well, the base assumption here (we'll need to verify) that anonymous access will be the only one or default one in the majority of the setups. And the idea is that Share plugin shouldn't show Public URL option during sharing/embedding if there is nothing special to be added to the URL that would make UI less loaded and confusing.

If an administrator changes the auth provider config, then they'd potentially invalidate any previously generated URLs if the anonymous provider suddenly isn't the default anymore.

Actually it feels reasonable to me to show login selector if the URL was shared without Public URL (was hidden when anonymous was default, so user didn't indicate that this URL should use anonymous access specifically) in case anonymous becomes non-default.

Do you see any issues here?

Copy link
Member

Choose a reason for hiding this comment

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

Well, the base assumption here (we'll need to verify) that anonymous access will be the only one or default one in the majority of the setups. And the idea is that Share plugin shouldn't show Public URL option during sharing/embedding if there is nothing special to be added to the URL that would make UI less loaded and confusing.

I'd personally be surprised if anonymous access was commonly the only provider, but I take your point here.

Actually it feels reasonable to me to show login selector if the URL was shared without Public URL (was hidden when anonymous was default, so user didn't indicate that this URL should use anonymous access specifically) in case anonymous becomes non-default.

I'm torn on this. From a security perspective, it makes sense to fail closed if the auth config changes in a way that alters the default provider. On the other hand, the administrator has to take explicit action to make the anonymous provider the default/only provider, so you could argue that generating these share links is intentionally making them available anonymously.

Let's keep it the way you have it for now. This is something we can easily change if we decide to take a different approach

}

this.httpAuthorizationHeader = AnonymousAuthenticationProvider.createHTTPAuthorizationHeader(
Copy link
Member

Choose a reason for hiding this comment

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

❤️

} catch (err) {
const errorMessage = getDetailedErrorMessage(err);
if (getErrorStatusCode(err) === 401) {
logger.error(`Anonymous access may not be properly configured yet: ${errorMessage}`);
Copy link
Member

Choose a reason for hiding this comment

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

nit: warn might be a better severity here.

@@ -51,8 +67,23 @@ export class SecurityOssPlugin
}

public start(core: CoreStart) {
const isAnonymousAccessEnabled = !!core.application.capabilities.security?.anonymousAccess;
Copy link
Member

Choose a reason for hiding this comment

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

I think capabilities are more OK than endpoints, but I don't know if we've codified this anywhere.

I don't love the idea of using x-pack endpoints in OSS, because that could lead to 404 errors, which is sloppy. I see you've guarded against that here, but I worry about the precedent this sets.

For the insecure cluster warning, we defined the endpoint in OSS and allowed x-pack security to augment the logic. This allows the endpoint to work in all distributions, while still allowing for distribution-specific behavior.

/**
* Defines Security OSS application state.
*/
export interface AppState {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: couldn't find a better name for that service, data and endpoint, kind of analogy with ASP.NET view state, but happy to take any other name if you have suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with AppState here. That makes sense to me

@azasypkin
Copy link
Member Author

Thanks for the feedback @legrego ! I took a stub at implementing changes we discussed yesterday, turned out a bit more complex than I expected, but it seems to do the job. Would you mind taking a look whenever you have time and let me know if it corresponds to what you had in mind?

@@ -84,37 +86,30 @@ export class InsecureClusterService {
};
}

public start({ core }: StartDeps): InsecureClusterServiceStart {
const shouldInitialize =
Copy link
Member Author

Choose a reason for hiding this comment

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

note: We can keep this check here if you wish, but we have it in the AppState service now and the overhead we'll get here should be negligible in theory (unnecessary Observable subscription in initializeAlert)


export interface SecurityOssPluginSetup {
insecureCluster: InsecureClusterServiceSetup;
}

export interface SecurityOssPluginStart {
insecureCluster: InsecureClusterServiceStart;
anonymousAccess: {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: I'll discuss the final API shape with AppArch, but I don't expect it to change too much. It feels the best option would be to give Share plugin consumers entire capabilities object so that they can the check on their own.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, I don't see a need to filter out the capabilities object on our side.

}

export class SecurityOssPlugin implements Plugin<SecurityOssPluginSetup, void, {}, {}> {
private readonly config$: Observable<ConfigType>;
private readonly logger: Logger;

private anonymousAccessServiceProvider?: () => AnonymousAccessService;
Copy link
Member Author

Choose a reason for hiding this comment

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

note: kind of clumsy, but I cannot think of a better approach to pass service that is only available at start.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. It's at least consistent with the way we've done this elsewhere 🤷‍♂️

@legrego legrego self-requested a review January 6, 2021 15:54
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Thanks for the edits! The high-level approach here looks good to me, and I think this will work really well for the sharing use case

start({ core }: StartDeps): AppStateServiceStart {
const appStatePromise = core.http.anonymousPaths.isAnonymous(window.location.pathname)
? Promise.resolve(DEFAULT_APP_STATE)
: core.http.get<AppState>('/internal/security_oss/app_state').catch(() => DEFAULT_APP_STATE);
Copy link
Member

Choose a reason for hiding this comment

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

🏅 thanks for falling back with .catch()


if (!this.isAnonymousAccessEnabled) {
this.logger.warn('Anonymous access is not enabled');
return DEFAULT_CAPABILITIES;
Copy link
Member

Choose a reason for hiding this comment

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

We recently introduced the concept of "default capabilities" into core, so I think we should instead use that mechanism here. This will allow us to return the full set of UI capabilities, so consumers won't have to worry as much about undefined properties on the capabilities object.

We'll need to update the resolveCapabilities signature to allow for an optional useDefaultCapabilities boolean, which we can pass through to the underlying function which currently defaults to false.

So the call here would instead look something like:

Suggested change
return DEFAULT_CAPABILITIES;
return capabilities.resolveCapabilities(fakeRequest, true);

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, interesting, didn't know that. It sounds like a way to go then, but it can be a bit tricky for the case when anonymous service account isn't properly configured and we'll get 401. If I understand everything correctly we have 3 different cases here:

  • Anonymous access is not enabled:
const fakeAnonymousRequest = KibanaRequest.from({ auth: { isAuthenticated: false } });
const caps = await capabilities.resolveCapabilities(fakeAnonymousRequest, { useDefaultCaps: true });
  • Anonymous access is enabled and properly configured:
const fakeAnonymousRequest = KibanaRequest.from({ auth: { isAuthenticated: true } });
const caps = await capabilities.resolveCapabilities(fakeAnonymousRequest);
  • Anonymous access is enabled but misconfigured (we don't know that in advance):
const fakeAnonymousRequest = KibanaRequest.from({ auth: { isAuthenticated: true } });
const caps = await capabilities.resolveCapabilities(fakeAnonymousRequest);

// if we get 401 then we'll try again this:

const fakeAnonymousRequest = KibanaRequest.from({ auth: { isAuthenticated: false } });
const caps = await capabilities.resolveCapabilities(fakeAnonymousRequest, { useDefaultCaps: true });

For the last case we could also make an additional "pre-flight" request to _authenticate and depending on the result switch useDefaultCaps and isAuthenticated on or off. It may be more correct solution, but I'm not sure if it makes sense to optimize for the case that should be very rare at the cost of an additional request for all cases.

What do you think? I'll go ahead without pre-flight request to _authenticate for now, but can quickly adjust.

Copy link
Member

Choose a reason for hiding this comment

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

I think you summarized the scenarios correctly here. For the misconfigured case, I don't have a strong opinion either way - _authenticate does feel more correct, so I'm +1 as long as it's not too difficult to implement.

{ path: '/internal/security_oss/anonymous_access/capabilities', validate: false },
async (_context, request, response) => {
return response.ok({
body: await getAnonymousAccessService().getCapabilities(request),
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this route is always expecting the anonymous access service to be defined, but since it's only registered by x-pack security (as far as I can see), I think this will throw an exception for the OSS distribution whenever its called.

Copy link
Member Author

@azasypkin azasypkin Jan 7, 2021

Choose a reason for hiding this comment

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

Yeah, it will, and user will get 500 error. Would you like it to have some default 200 behavior (then we'll have to have default capabilities in two places or return null/empty object) or just more appropriate error code (e.g. 501 Not Implemented or 403 Forbidden)?

Copy link
Member

Choose a reason for hiding this comment

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

I like 501 Not Implemented for this case. I assume that this endpoint won't normally be called if anonymous access is disabled (and by extension, when called in the OSS distribution)? In other words, we'll have enough information client-side so that we know not to even attempt the call

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume that this endpoint won't normally be called if anonymous access is disabled (and by extension, when called in the OSS distribution)?

Correct, there shouldn't be any case when Kibana would call it with disabled anonymous access.

}

export class SecurityOssPlugin implements Plugin<SecurityOssPluginSetup, void, {}, {}> {
private readonly config$: Observable<ConfigType>;
private readonly logger: Logger;

private anonymousAccessServiceProvider?: () => AnonymousAccessService;
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. It's at least consistent with the way we've done this elsewhere 🤷‍♂️

/**
* Defines Security OSS application state.
*/
export interface AppState {
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with AppState here. That makes sense to me


export interface SecurityOssPluginSetup {
insecureCluster: InsecureClusterServiceSetup;
}

export interface SecurityOssPluginStart {
insecureCluster: InsecureClusterServiceStart;
anonymousAccess: {
Copy link
Member

Choose a reason for hiding this comment

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

I agree, I don't see a need to filter out the capabilities object on our side.

@azasypkin azasypkin added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement feedback_needed labels Jan 11, 2021
@azasypkin azasypkin marked this pull request as ready for review January 11, 2021 13:57
@azasypkin azasypkin requested review from a team as code owners January 11, 2021 13:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM for core changes

Comment on lines 165 to 181
resolveCapabilities: (request) => this.resolveCapabilities(request, [], false),
resolveCapabilities: (request, options) =>
this.resolveCapabilities(request, [], !!options?.useDefaultCapabilities),
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: imho options?.useDefaultCapabilities ?? false is slightly more explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, agree. I'll change, thanks!

@legrego
Copy link
Member

legrego commented Jan 13, 2021

@elasticmachine merge upstream

@legrego
Copy link
Member

legrego commented Jan 14, 2021

I'll try to finish reviewing this tomorrow 👍

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM with some optional nits - tested locally, and all seems to be working great!

return anonymousAccess.accessURLParameters;
},
async getCapabilities() {
return await core.http.get<Capabilities>(
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: unnecessary await

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there was a verbal agreement in a Platform team long ago to do this since we don't define return type explicitly here and it's not always obvious that function returns Promise. I couldn't find it recorded anywhere, so will remove it.

const appState: AppState = {
insecureClusterAlert: { displayAlert },
anonymousAccess: {
isEnabled: !!anonymousAccessService?.isAnonymousAccessEnabled,
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: similar to #87091 (comment):

Suggested change
isEnabled: !!anonymousAccessService?.isAnonymousAccessEnabled,
isEnabled: anonymousAccessService?.isAnonymousAccessEnabled ?? false,

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I need to get used to this better/more readable alternative!

}

// We should use credentials of the anonymous service account instead of credentials of the
// current user to figure out if the specified Saved Object type can be accessed anonymously.
Copy link
Member

Choose a reason for hiding this comment

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

nit: the comment about "specified Saved Object type" isn't relevant anymore, now that we're performing a capabilities check instead of an explicit Saved Object Type check

authenticateRequest && this.httpAuthorizationHeader
? { authorization: this.httpAuthorizationHeader.toString() }
: {},
// This flag is essential for the security capability switcher that relies on it to decide if
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the comment here 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securityOss 8 10 +2

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securityOss 11.6KB 13.1KB +1.5KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@azasypkin azasypkin merged commit 951aa66 into elastic:master Jan 15, 2021
@azasypkin azasypkin deleted the issue-83650-anonymous-improve-ux branch January 15, 2021 09:19
azasypkin added a commit to azasypkin/kibana that referenced this pull request Jan 15, 2021
@azasypkin
Copy link
Member Author

7.x/7.12.0: 93db245

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:Security/Authentication Platform Security - Authentication Feature:Security/Authorization Platform Security - Authorization release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants