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

disable Basic Auth to remove dependency on deprecated crypt library #1165

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

peterbecich
Copy link
Member

@peterbecich peterbecich commented Jan 16, 2023

Closes: #1153

This PR disables Basic Auth, per this comment: #1153 (comment) Basic Auth is disabled in the simplest possible way, by returning a UnrecognizedAuthError. More could be done to completely delete Basic Auth. Is this sufficient for now?

hackage-server builds successfully without the crypt library.

I have barely tested this; I am able to log in with the "admin" account on Hackage running locally.

@peterbecich peterbecich marked this pull request as ready for review January 16, 2023 08:22
@gbaz
Copy link
Contributor

gbaz commented Jan 16, 2023

I think it would be better to get rid of that file entirely and change here (

checkAuthenticated :: ServerMonad m => RealmName -> Users.Users -> m (Either AuthError (UserId, UserInfo))
) to return a response about basic auth not being supported any longer.

@peterbecich
Copy link
Member Author

@gbaz , do you want checkTokenAuth removed, as well?

checkAuthenticated :: ServerMonad m => RealmName -> Users.Users -> m (Either AuthError (UserId, UserInfo))
checkAuthenticated realm users = do
req <- askRq
return $ case getHeaderAuth req of
Just (DigestAuth, ahdr) -> checkDigestAuth users ahdr req
Just _ | plainHttp req -> Left InsecureAuthError
Just (BasicAuth, ahdr) -> checkBasicAuth users realm ahdr
Just (AuthToken, ahdr) -> checkTokenAuth users ahdr
Nothing -> Left NoAuthError

@peterbecich
Copy link
Member Author

@andreasabel , would you review this? Is it adequate as is? We can further remove the old auth system later IMO.

Merging this will unblock #1154

@gbaz gbaz merged commit 9774da5 into haskell:master Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove LegacyPasswd code and dependency on crypt
2 participants