WordPress.org

Make WordPress Core

Opened 7 weeks ago

Closed 7 weeks ago

Last modified 6 weeks ago

#51939 closed defect (bug) (fixed)

Basic Auth staging protections conflicts with App Passwords

Reported by: TimothyBlynJacobs Owned by: TimothyBlynJacobs
Milestone: 5.6 Priority: highest omg bbq
Severity: blocker Version: 5.6
Component: Application Passwords Keywords: has-patch has-unit-tests dev-reviewed
Focuses: rest-api Cc:

Description

Application Passwords uses Basic Authentication to pass the username and password authentication data.

The first time is_user_logged_in is called during WP_REST_Server::serve_request(), we attempt authentication. If the PHP_AUTH_USER variable isn't populated, we don't attempt App Passwords authentication at all. If it is present, we attempt to authentication based on it. If an error is encountered, this is stored and then returned via the filter in WP_REST_Server::check_authentication and the rest_application_password_check_errors callback.

This becomes an issue if the site is globally behind Basic Authentication of its own. Because now the REST API will see the Basic Auth credentials, and try to authenticate the user even if it isn't intended. For reference: https://wordpress.org/support/topic/sitewide-basic-authentication-and-application-password-causing-issues/#post-13745874

There are a couple of different possible solutions.

There are two goals I'd like to maintain. For it to be a standards based as possible, and maintain the friendliest DUX. A developer should be able to tell that there was an issue with authentication.

  1. Switch from using the Authorization header to something like WP-Authorization. It would follow the same format, but since it is a different header, there would be no chance of conflict. It would also allow for using App Passwords when a site is behind Basic Auth. This has a number of issues though since middleware wouldn't know that this is an authenticated request anymore. Particularly since we also aren't passing any cookies. It also breaks DUX of clients that have support for basic auth built in.
  1. Instead of a whole different header, we can use a different authentication scheme. Such as WP-App-Password {token} instead of Basic {token}. This alleviates the issues due to the Authorization header not being used, but we still have the DUX issues.
  1. Don't report errors if the password isn't exactly 24 characters, ie the format of App Passwords. Since App Passwords are unlikely to be typed by hand, and even if they were, an Application could reject them if they weren't 24 characters, this would be a fairly good indicator that App Passwords were attempted to be used. But if the server level Basic Auth password was 24 characters long, you'd also have this issue.
  1. Require the client to prefix the App Password with something like wpapp. This is a more surefire signifier than the password being 24 characters, but it would require clients to juggle adding the prefix or not for existing App Passwords. Versus some of the following options, this has the advantage of the signifier being colocated with the other authentication data.

The following solutions would all also return authentication errors in a separate response header. For instance, we could send X-WP-Auth-Failed: {code}={message} or something.

  1. That's it. Never return an authentication error in the main body of the response.
  1. Require a signifier in the request to return an authentication error, if it is missing don't return the error from rest_application_password_check_errors(). For instance, a client could always pass a X-WP-App-Passwords: true header. This is quite simple for a developer to include.
  1. If the REST API response is a 401 Unauthorized, see rest_authorization_required_code, return the authentication error if any. The 401 status code indicates that we tried to authenticate you as a user, but we couldn't determine who you were. Versus a 403 indicating you didn't have sufficient capabilities.

This would be implemented something like this.

<?php
$authenticated = $this->check_authentication();

if ( ! is_wp_error( $authenticated ) || rest_application_password_collect_status( null ) === $authenticated ) {
        $result = $this->dispatch( $request );
} else {
        $result = $authenticated;
}

$response = $this->dispatch();

// This indicates an error due to the user being logged out instead of
// not having the right caps. See rest_authorization_required_code().
if ( $response->get_status() === 401 && is_wp_error( $authenticated ) ) {
        $response = $authenticated;
}

There are some disadvantages to not returning the authentication error immediately. Let's say you are submitting a comment. If anonymous comments are on, the comments controller doesn't require you to be logged in. So you may end up submitting a comment anonymously even if you were trying to submit it as a logged in user. A similar thing could happen for the route that was originally reported, a contact form submission.

Of note, we somewhat already have the anonymous issue with cookie auth. If the nonce is missing, no error is returned. While this is very confusing to developers who are learning the REST API for the first time, once the coding error is fixed an error is always returned. Whereas with App Passwords the error is more of a runtime issue than a coding logic issue, the App Password might be invalid or deleted.

If at all possible, I'd like to be able to return the authentication error in the body of the response. It is clearer to clients that an authentication error happened versus putting that information in the headers of the response. It also is a higher fidelity transfer mechanism than the response headers. We report the $data included in the WP_Error object returned.

My current preference I think is option 7. This lets us still be standards based and the vast majority of the time would allow for us to return the authentication error in the body of the response. That wouldn't catch the issue with routes that can be anonymous, if we want to also solve that issue I would prefer option 6.

Change History (24)

#1 @georgestephanis
7 weeks ago

Also, if the site itself is accessed via basic auth, maybe we could detect that in wp-admin and set an option disabling application passwords in the first place?

Last edited 7 weeks ago by georgestephanis (previous) (diff)

#2 @TimothyBlynJacobs
7 weeks ago

Of note, we have to protect against people using rest_authentication_errors to report capability issues versus just reporting on the state of the authentication attempt. In other words Authentication vs Authorization. This is why we are specifically checking if that was an app passwords generated error.

#3 follow-up: @TimothyBlynJacobs
7 weeks ago

Also, if the site itself is accessed via basic auth, maybe we could detect that and set an option disabling application passwords in the first place?

Ooh nice. I like how deviously simple this would be. Where would we put that logic? Perhaps detect that on wp_loaded()? We'd need to regularly invalidate it and make sure that they weren't passing it to a REST API route.

#4 in reply to: ↑ 3 @georgestephanis
7 weeks ago

Replying to TimothyBlynJacobs:

Also, if the site itself is accessed via basic auth, maybe we could detect that and set an option disabling application passwords in the first place?

Ooh nice. I like how deviously simple this would be. Where would we put that logic? Perhaps detect that on wp_loaded()? We'd need to regularly invalidate it and make sure that they weren't passing it to a REST API route.

I have no opinions to when the check this. I'd shy away from dropping it in a transient in case the transient expires when unauthed api requests are still happening?

#5 @TimothyBlynJacobs
7 weeks ago

I agree, we should put it in an option somewhere. Should it be network wide or per-site?

We could put it in the beginning of admin.php like how we handle the upgrade checks.

Ideally, we'd also be able to handle if the user is auto-updated when they aren't visiting the admin. Depending on how the auto update happens, the basic auth header might not be populated in upgrade_560. For instance if cron is fired out of band, isn't protected by basic auth at all, or the host uses WP-CLI to update.

We could potentially do a loopback request to the homage page and check for a 401 error or WWW-Authenticate header in the response? Loopback doesn't always work, but we've been reporting that error in Site Health for a while now, so maybe this isn't a major issue.

We also need to handle invalidating this. Would there be an issue checking for PHP_AUTH_USER on every admin.php and deleting it immediately? Or should we only delete it if it has been let's say 24 hours of it not being reported/

We should also report that App Passwords is disabled in Site Health due to this in Site Health, but this can wait until 5.6.1.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 weeks ago

This ticket was mentioned in PR #790 on WordPress/wordpress-develop by TimothyBJacobs.


7 weeks ago

  • Keywords has-patch added

Don't attempt App Passwords auth if the site hasn't generated any app passwords.

Show an error message if trying to create an App Password and the Basic Auth header is in use.

Trac ticket: https://core.trac.wordpress.org/ticket/51939

This ticket was mentioned in Slack in #core-passwords by timothybjacobs. View the logs.


7 weeks ago

#9 @TimothyBlynJacobs
7 weeks ago

After discussion in #core-passwords we decided on a different solution.

We now won't return any errors from wp_authenticate_application_password if there are no App Passwords created. Additionally, we prevent creating new App Passwords if it looks like the site is already behind Basic Authentication.

#10 @adamsilverstein
7 weeks ago

I was able to test this in my local setup and verify both the issue and that the proposed fix in the PR works as expected:

  • After setting up local auth and submitting a form (using CF7 as described in the issue) as a non-logged in user, I received an authentication error from the rest API. Status 401 message: "Unknown username. Check again or try your email address."
  • Testing the PR the form submission/endpoint works as expected.
  • Trying to set up Application Password when my site is behind basic auth, I discover I cannot:

https://p201.p0.n0.cdn.getcloudapp.com/items/8LunjXne/Image%202020-12-04%20at%2012.36.56%20PM.jpg

#11 @hellofromTonya
7 weeks ago

  • Keywords has-unit-tests added

#12 @helen
7 weeks ago

  • Keywords has-unit-test dev-reviewed added; has-unit-tests removed

Patch looks good to me and this seems like a good approach for 5.6.

#13 @helen
7 weeks ago

  • Keywords has-unit-tests added; has-unit-test removed

#14 @TimothyBlynJacobs
7 weeks ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 49752:

App Passwords: Prevent conflicts when Basic Auth is already used by the site.

Application Passwords uses Basic Authentication to transfer authentication details. If the site is already using Basic Auth, for instance to implement a private staging environment, then the REST API will treat this as an authentication attempt and would end up generating an error for any REST API request.

Now, Application Password authentication will only be attempted if Application Passwords is in use by a site. This is flagged by setting an option whenever an Application Password is created. An upgrade routine is added to set this option if any App Passwords already exist.

Lastly, creating an Application Password will be prevented if the site appears to already be using Basic Authentication.

Props chexwarrior, georgestephanis, adamsilverstein, helen, Clorith, marybaum, TimothyBlynJacobs.
Fixes #51939.

#15 @helen
7 weeks ago

In 49754:

App Passwords: Prevent conflicts when Basic Auth is already used by the site.

Application Passwords uses Basic Authentication to transfer authentication details. If the site is already using Basic Auth, for instance to implement a private staging environment, then the REST API will treat this as an authentication attempt and would end up generating an error for any REST API request.

Now, Application Password authentication will only be attempted if Application Passwords is in use by a site. This is flagged by setting an option whenever an Application Password is created. An upgrade routine is added to set this option if any App Passwords already exist.

Lastly, creating an Application Password will be prevented if the site appears to already be using Basic Authentication.

Props chexwarrior, georgestephanis, adamsilverstein, helen, Clorith, marybaum, TimothyBlynJacobs.
Reviewed by TimothyBlynJacobs, helen.
Merges [49752] to the 5.6 branch.
Fixes #51939.

#16 @prbot
7 weeks ago

TimothyBJacobs commented on PR #790:

Merged in 38361be.

This ticket was mentioned in Slack in #core by helen. View the logs.


7 weeks ago

#18 @spacedmonkey
6 weeks ago

After reviewing this patch, as the maintainer of multisite, I have one simple recommendation, save the OPTION_KEY_IN_USE against the main network. This can be done like this.

update_network_option( get_main_network_id(), WP_Application_Passwords::OPTION_KEY_IN_USE, 1 );
return (bool) get_network_option(  get_main_network_id(), self::OPTION_KEY_IN_USE );

This adds a key benefit, if you are using multi network, then this option is stored locally on the main network. Using the main network's option as a global store is a pattern already found in core.

CC other multisite maintainers @jeremyfelt @johnjamesjacoby @flixos90

This ticket was mentioned in Slack in #core-passwords by spacedmonkey. View the logs.


6 weeks ago

This ticket was mentioned in PR #793 on WordPress/wordpress-develop by spacedmonkey.


6 weeks ago

Small fix to save detection of application password support to the main network to improve support for multi network support.

Trac ticket: https://core.trac.wordpress.org/ticket/51939

#21 @prbot
6 weeks ago

spacedmonkey commented on PR #793:

CC @TimothyBJacobs

#22 @spacedmonkey
6 weeks ago

In #793 I had a simple patch to better support WordPress multi network.

Detecting a feature like is application password supported, is a global feature not a per network feature. So if application password is set, should be stored globally like site meta support.

#23 @TimothyBlynJacobs
6 weeks ago

In 49764:

App Passwords: Store the "in use" option in the main network options.

Whether App Passwords are being used is a global featurel, not a per-network feature. This fixes issues on Multi Network installs if App Passwords are used on a different network from where they were created.

Props spacedmonkey.
Fixes #51939.
See [49752].

#24 @helen
6 weeks ago

In 49765:

App Passwords: Store the "in use" option in the main network options.

Whether App Passwords are being used is a global featurel, not a per-network feature. This fixes issues on Multi Network installs if App Passwords are used on a different network from where they were created.

Props spacedmonkey.
Fixes #51939.
See [49752].
Merges [49764] to the 5.6 branch.

Note: See TracTickets for help on using tickets.