cedaro / satispress

Expose installed WordPress plugins and themes as Composer packages.
508 stars 51 forks source link

Consider using API keys for default authentication instead of user credentials #57

Closed bradyvercher closed 6 years ago

bradyvercher commented 6 years ago

Basic authentication comes with security issues as currently detailed in the documentation. Mitigating those issues requires jumping through a few hurdles that are easier to ignore than implement, especially for people not aware of issues:

Using credentials for a WordPress user makes it too easy to accidentally leak passwords and conditions users to provide their credentials to third-parties, making them susceptible to phishing attacks or dependent on the security of the third-party who needs to store the password in clear text for future use.

To work around these issues, each user or client that accesses SatisPress needs a separate user account with limited capabilities. This requires manually assigning those capabilities to an existing role, which potentially opens SatisPress up to all users on a system, or creating a new role. That's a lot of work for basic security, and while it might be OK for a single user, it would quickly become a pain for a team of developers or vendors looking to add support for customers.

With those problems outlined, a new solution should:

While Composer has support for authentication schemes specific to popular services (GitHub, GitLab, Bitbucket, tc ), support appears to be limited to Basic Auth for general use. It might be possible to write a Composer plugin to support other auth methods, but that requires an additional step for consumers to require it.

At this point, I'm leaning toward using API keys with Basic Auth rather than WP user credentials with the following workflow:

The benefits are:

This approach can be further extended by allowing each user to have one or more API keys for each client that needs to access SatisPress. Revoking access would just require deleting an API key and wouldn't affect other clients.

API keys are still susceptible to being discovered if used over plain HTTP, but Composer is configured to only work with HTTPS by default and other auth schemes that are better suited for plain HTTP wouldn't work well with Composer without a plugin.

I'm guessing this would likely break backward compatibility for sites using the pre-0.3.0 Basic Auth implementation, but given the security benefits, I think it would be worthwhile.

GaryJones commented 6 years ago

I'm +1 for this, with one small cavaet to:

When Composer prompts for the credentials to access a SatisPress repository, the user will enter the API key as the username and satispress as the password

I would say to make the password the same as the API key. Notes:

Question:

bradyvercher commented 6 years ago

The API key is essentially just an identifier for a user, which is why I figured it would work well in the username field. A lot of implementations that use API keys over Basic Auth just leave the password blank or use a header with the key as the value. A single, unique identifier is usually all that's available when using API keys.

It's not ideal for anything sensitive, but the API is read-only and nothing is overly sensitive. Whether the API key or satispress is used for the password won't affect security one way or another since they're both known values.

Since this wouldn't be using a custom authentication scheme or header, using satispress for the password would allow us to identify the the request as being for a SatisPress resource, otherwise, if another Basic Auth implementation is enabled on the same site, they could conflict with each other. I'll have to do some testing for that, though.

The edit_users capability should probably work for creating API keys.

I've written a few authentication providers and this basically just reworks the existing Basic auth implementation, so it shouldn't take much effort. I'd rather get it in the next release, otherwise it could break backward compatibility for anyone that gets set up with the current approach.