auth icon indicating copy to clipboard operation
auth copied to clipboard

Token should be encrypted

Open jdmcd opened this issue 7 years ago • 6 comments

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

jdmcd avatar Mar 19 '19 22:03 jdmcd

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.

tanner0101 avatar Mar 20 '19 02:03 tanner0101

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

jdmcd avatar Mar 20 '19 02:03 jdmcd

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

0xTim avatar Mar 21 '19 12:03 0xTim

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.

MrMage avatar Apr 02 '19 17:04 MrMage

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
}

jdmcd avatar Apr 08 '19 04:04 jdmcd

Looks pretty good to me!

0xTim avatar Apr 08 '19 07:04 0xTim