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

Thoughts and Best Practices in Desiging API Access Token #4793

Open
beeonthego opened this issue Aug 26, 2018 · 2 comments
Open

Thoughts and Best Practices in Desiging API Access Token #4793

beeonthego opened this issue Aug 26, 2018 · 2 comments
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@beeonthego
Copy link
Contributor

beeonthego commented Aug 26, 2018

Thank you all contributors for making this open source project.

We are building an app that will use Gitea at the backend. The App will primarily access gitea through API. We would like the app to be able to scale, and would like to check access token at API gateway before the requests hit gitea instance. Ideally the decision can be made at API gateway without db query.

This is not possible with the current implementation of gitea access token, and refactoring of access token data model is required. Here are our initial thoughts on the design of the API access token while maintaining backward compatibility. Before submitting a pull request, we would like to hear the thoughts and best practices of people using the project now.

Use JWT Bearer Token as API access token

The bearer token carries extra claims that can be checked on a different machine, using shared secret server key.

Care must be taken to implement JWT access token properly.

Security

The overall design principle is that in case a database is compromised, the damage is contained to the information loss only and no further privilege can be obtained.

To address the security concerns raised in issue #3789, the token can be signed with a strong and permanent server key. The key should be long enough and beyond the possibility of brute force attack.

The key name and claims can be stored in the database to track what keys have been issued, and to be used in key admin UI. However only the API keys signed with server secret will be able to access the API. The API is not accessible using the information from the database alone.

The key can be set as environment variable, so gitea admin can decide the key length depending on their needs for security and performance, with a minimum of 256 bits (32 bytes).

The claims can be signed with a single algorithm (eg HS256) to avoid the flaws of JWT (eg tampering on signing algorithm and signature). This is also how drone signs access token now.

Backward Compatibility

In order for the issued access tokens to continue to work (eg drone integration), the implementation can check the length of access token. if it is exactly 40 bytes long (sha1 hash), the current logic is applied (retrieving the key from db and check permissions). If the token is longer than 40 bytes, then new logic will be used.

When users log into Drone instance using gitea password, Drone will automatically retrieve the new access token and save it in drone db for future use. The transition will be seamless, and can take whatever time it requires.

data storage

The new claims can be added on top of the existing fields so the table can hold both the old issued tokens, and new information. This way old access tokens will continue to work until revoked or replaced with new tokens.

other benefits of using JWT bearer token

Users can decide to grant full access, or partial access to a token, eg by repo, api routes (regular expression), request methods. Details of the claims are to be discussed.
[edit]
With JWT and hs256 signing algorithm in place, it is possible to store the claims i/o access token in Drone db, and use the same secret server key on drone to sign the access token before making api requests to gitea server. This involves some code change in Drone server, which is easy to customize and implement. This storage strategy in drone gives peace of mind in case the drone db is compromised. People can't do much with the claims only found in drone db.

proposed composite signing key (u.Rands + u.Salt + hash with fixed server secret)

The disadvantage of signing token with a fixed server secret is that token is tied with a server secret. In order to revoke a token, a new server secret must be used thus revoking all tokens. This may not be realistic in multi user environment. The same problem exists with expiring tokens and refreshing tokens. Refreshing token is valid for a long time. A new server secret is required in order to revoke a lost refreshing token.

To maintain security and flexibility, we propose to sign tokens with a composite key including some variable strings per user, plus a hash with server secret. For example, it is possible to sign access token with individual user's rands and salts + hash.

The disadvantage is that a db query is required in order to check the validity of access token. Scaling an application with this approach means scaling the database at the same time.

This can be offered as an option and turned on with an environment variable. If true, then each token will be signed and checked using individual composite keys instead of a permanent server key. Otherwise, a fixed server secret will be used for signing.

We are writing the code to implement this internally. Collaboration on XORM, UI and test suites are welcome.

Your thoughts and suggestions?

@lunny lunny added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Aug 26, 2018
@beeonthego
Copy link
Contributor Author

Just submitted a PR #4806 to have some custom grants/claims parameters code to look at. comments are welcome.

@beeonthego
Copy link
Contributor Author

Here are the proposed changes to the api AccessToken struct. Can someone work on this and make a pull request to add claims column in db? Our code depends on this change in data structure.

When you prepare db migration, please update the values of this column in existing tokens to an empty string. gitea will panic if this column is left as NULL. thanks.

type AccessToken struct {
	ID   int64 `xorm:"pk autoincr"`
	UID  int64 `xorm:"INDEX"`
	Name string
	Sha1 string `xorm:"UNIQUE VARCHAR(40)"`

	CreatedUnix       util.TimeStamp `xorm:"INDEX created"`
	UpdatedUnix       util.TimeStamp `xorm:"INDEX updated"`
	HasRecentActivity bool           `xorm:"-"`
	HasUsed           bool           `xorm:"-"`

        // new field, to store in db as JSON, work with server secret to reconstruct the token
        // on the fly. Claims are stored in db. 
	Claims    	*TokenClaims 	 `xorm:"TEXT"` 
        // new field to hold jwt in memory, do not save jwt in db
        Content 	  string 	  `xorm:"-"` 
}

// optional restrict access by owner/org, repo, branch, route, http method.
type TokenClaims struct {
	Owner []string `json:"owner,omitempty"`
	Repo []string `json:"repo,omitempty"`
	Branch []string `json:"branch,omitempty"` //wildcard match
	Route []string `json:"route,omitempty"`  //wildcard match
	Method []string `json:"method,omitempty"`
	jwt.StandardClaims
	UID int64 `json:"uid,omitempty"` //user id
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

No branches or pull requests

2 participants