Skip to content
This repository has been archived by the owner on Nov 16, 2020. It is now read-only.

Token should be encrypted #64

Open
jdmcd opened this issue Mar 19, 2019 · 6 comments
Open

Token should be encrypted #64

jdmcd opened this issue Mar 19, 2019 · 6 comments
Labels
enhancement New feature or request
Projects

Comments

@jdmcd
Copy link
Member

jdmcd commented Mar 19, 2019

To prevent a database dump from revealing access to user accounts.

@tanner0101 tanner0101 added the enhancement New feature or request label Mar 20, 2019
@tanner0101 tanner0101 added this to To Do in Vapor 4 via automation Mar 20, 2019
@tanner0101
Copy link
Member

tanner0101 commented Mar 20, 2019

I wonder if we should hash the token instead? It should be fairly easy to just store the SHA256 digest and compare that instead of the token itself. It would be a bit harder if we wanted to use BCrypt, since we'd need a second field to lookup the token for verification. But I think SHA is the better digest algorithm here anyway since you want token lookup to be relatively quick.

@jdmcd
Copy link
Member Author

jdmcd commented Mar 20, 2019

Oh yeah good point, I think that’s the way to go. Shouldn’t be much of a speed difference either.

@0xTim
Copy link
Member

0xTim commented Mar 21, 2019

There probably needs to be a distinction between different types of tokens. Short lived tokens probably don't need to be encrypted since the risk of a database dump having damage is low (tokens with a lifetime of a hour etc)

Long-lived tokens (especially refresh tokens) should probably be stored properly - this means not SHA256 since it would be (relatively) easy to crack etc

@MrMage
Copy link

MrMage commented Apr 2, 2019

Long-lived tokens (especially refresh tokens) should probably be stored properly - this means not SHA256 since it would be (relatively) easy to crack etc

SHA256 should be fine if the tokens are randomly generated string of sufficient length (say, 192 or 256 bits). Salting is only required to generate additional entropy, which is already provided by the full token being random. And BCrypt is only needed to make attacks on a small-ish solution set infeasible; brute-forcing 2^256 possible values is of course intractable even without BCrypt. See e.g. https://security.stackexchange.com/a/122855/177736.

@jdmcd
Copy link
Member Author

jdmcd commented Apr 8, 2019

How does this look for now? Just to get something into my project without requiring breaking changes:

extension Token: BearerAuthenticatable, Authentication.Token {
    func willCreate(on conn: MySQLConnection) throws -> EventLoopFuture<Token> {
        token = try SHA256.hash(token).base64EncodedString()
        return conn.future(self)
    }
    
    static func authenticate(using bearer: BearerAuthorization, on connection: DatabaseConnectable) -> Future<Token?> {
        guard let hashed = try? SHA256.hash(bearer.token).base64EncodedString() else {
            return connection.future(error: Abort(.internalServerError))
        }
        
        return Token.query(on: connection).filter(tokenKey == hashed).first()
    }

    static var userIDKey: WritableKeyPath<Token, Int> {
        return \Token.user_id
    }
    
    static var tokenKey: WritableKeyPath<Token, String> {
        return \Token.token
    }

    typealias UserType = User
}

@0xTim
Copy link
Member

0xTim commented Apr 8, 2019

Looks pretty good to me!

@tanner0101 tanner0101 moved this from Backlog to Done in Vapor 4 Mar 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
Vapor 4
  
Done
Development

No branches or pull requests

4 participants