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

feat: cover 100% of crypto with tests #1892

Merged
merged 2 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ migrate_test: ## Run database migrations for test.

test: build ## Run tests.
go test $(CHECK_FILES) -coverprofile=coverage.out -coverpkg ./... -p 1 -race -v -count=1
./hack/coverage.sh

vet: # Vet the code
go vet $(CHECK_FILES)
Expand Down
21 changes: 21 additions & 0 deletions hack/coverage.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
FAIL=false

for PKG in "crypto"
do
UNCOVERED_FUNCS=$(go tool cover -func=coverage.out | grep "^github.com/supabase/auth/internal/$PKG/" | grep -v '100.0%$')
UNCOVERED_FUNCS_COUNT=$(echo "$UNCOVERED_FUNCS" | wc -l)

if [ "$UNCOVERED_FUNCS_COUNT" -gt 1 ] # wc -l counts +1 line
then
echo "Package $PKG not covered 100% with tests. $UNCOVERED_FUNCS_COUNT functions need more tests. This is mandatory."
echo "$UNCOVERED_FUNCS"
FAIL=true
fi
done

if [ "$FAIL" = "true" ]
then
exit 1
else
exit 0
fi
33 changes: 27 additions & 6 deletions internal/api/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,12 @@ import (
"time"

"github.com/gofrs/uuid"
"github.com/supabase/auth/internal/observability"
"github.com/sirupsen/logrus"
standardwebhooks "github.com/standard-webhooks/standard-webhooks/libraries/go"

"github.com/supabase/auth/internal/conf"
"github.com/supabase/auth/internal/crypto"

"github.com/sirupsen/logrus"
"github.com/supabase/auth/internal/hooks"

"github.com/supabase/auth/internal/observability"
"github.com/supabase/auth/internal/storage"
)

Expand Down Expand Up @@ -103,7 +101,7 @@ func (a *API) runHTTPHook(r *http.Request, hookConfig conf.ExtensibilityPointCon
}
msgID := uuid.Must(uuid.NewV4())
currentTime := time.Now()
signatureList, err := crypto.GenerateSignatures(hookConfig.HTTPHookSecrets, msgID, currentTime, inputPayload)
signatureList, err := generateSignatures(hookConfig.HTTPHookSecrets, msgID, currentTime, inputPayload)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -382,3 +380,26 @@ func (a *API) runHook(r *http.Request, conn *storage.Connection, hookConfig conf

return response, nil
}

func generateSignatures(secrets []string, msgID uuid.UUID, currentTime time.Time, inputPayload []byte) ([]string, error) {
hf marked this conversation as resolved.
Show resolved Hide resolved
SymmetricSignaturePrefix := "v1,"
// TODO(joel): Handle asymmetric case once library has been upgraded
var signatureList []string
for _, secret := range secrets {
if strings.HasPrefix(secret, SymmetricSignaturePrefix) {
trimmedSecret := strings.TrimPrefix(secret, SymmetricSignaturePrefix)
wh, err := standardwebhooks.NewWebhook(trimmedSecret)
if err != nil {
return nil, err
}
signature, err := wh.Sign(msgID.String(), currentTime, inputPayload)
if err != nil {
return nil, err
}
signatureList = append(signatureList, signature)
} else {
return nil, errors.New("invalid signature format")
}
}
return signatureList, nil
}
5 changes: 1 addition & 4 deletions internal/api/magic_link.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,7 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error {
if isNewUser {
// User either doesn't exist or hasn't completed the signup process.
// Sign them up with temporary password.
password, err := crypto.GeneratePassword(config.Password.RequiredCharacters, 33)
if err != nil {
return internalServerError("error creating user").WithInternalError(err)
}
password := crypto.GeneratePassword(config.Password.RequiredCharacters, 33)

signUpParams := &SignupParams{
Email: params.Email,
Expand Down
62 changes: 20 additions & 42 deletions internal/api/mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,7 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {

var url string
now := time.Now()
otp, err := crypto.GenerateOtp(config.Mailer.OtpLength)
if err != nil {
// OTP generation must always succeed
panic(err)
}
otp := crypto.GenerateOtp(config.Mailer.OtpLength)

hashedToken := crypto.GenerateTokenHash(params.Email, otp)

Expand Down Expand Up @@ -300,19 +296,18 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {
}

func (a *API) sendConfirmation(r *http.Request, tx *storage.Connection, u *models.User, flowType models.FlowType) error {
var err error

config := a.config
maxFrequency := config.SMTP.MaxFrequency
otpLength := config.Mailer.OtpLength

if err := validateSentWithinFrequencyLimit(u.ConfirmationSentAt, maxFrequency); err != nil {
if err = validateSentWithinFrequencyLimit(u.ConfirmationSentAt, maxFrequency); err != nil {
return err
}
oldToken := u.ConfirmationToken
otp, err := crypto.GenerateOtp(otpLength)
if err != nil {
// OTP generation must succeeed
panic(err)
}
otp := crypto.GenerateOtp(otpLength)

token := crypto.GenerateTokenHash(u.GetEmail(), otp)
u.ConfirmationToken = addFlowPrefixToToken(token, flowType)
now := time.Now()
Expand Down Expand Up @@ -342,11 +337,8 @@ func (a *API) sendInvite(r *http.Request, tx *storage.Connection, u *models.User
otpLength := config.Mailer.OtpLength
var err error
oldToken := u.ConfirmationToken
otp, err := crypto.GenerateOtp(otpLength)
if err != nil {
// OTP generation must succeed
panic(err)
}
otp := crypto.GenerateOtp(otpLength)

u.ConfirmationToken = crypto.GenerateTokenHash(u.GetEmail(), otp)
now := time.Now()
if err = a.sendEmail(r, tx, u, mail.InviteVerification, otp, "", u.ConfirmationToken); err != nil {
Expand Down Expand Up @@ -382,15 +374,12 @@ func (a *API) sendPasswordRecovery(r *http.Request, tx *storage.Connection, u *m
}

oldToken := u.RecoveryToken
otp, err := crypto.GenerateOtp(otpLength)
if err != nil {
// OTP generation must succeed
panic(err)
}
otp := crypto.GenerateOtp(otpLength)

token := crypto.GenerateTokenHash(u.GetEmail(), otp)
u.RecoveryToken = addFlowPrefixToToken(token, flowType)
now := time.Now()
if err = a.sendEmail(r, tx, u, mail.RecoveryVerification, otp, "", u.RecoveryToken); err != nil {
if err := a.sendEmail(r, tx, u, mail.RecoveryVerification, otp, "", u.RecoveryToken); err != nil {
u.RecoveryToken = oldToken
if errors.Is(err, EmailRateLimitExceeded) {
return tooManyRequestsError(ErrorCodeOverEmailSendRateLimit, EmailRateLimitExceeded.Error())
Expand Down Expand Up @@ -422,11 +411,8 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u
}

oldToken := u.ReauthenticationToken
otp, err := crypto.GenerateOtp(otpLength)
if err != nil {
// OTP generation must succeed
panic(err)
}
otp := crypto.GenerateOtp(otpLength)

u.ReauthenticationToken = crypto.GenerateTokenHash(u.GetEmail(), otp)
now := time.Now()

Expand All @@ -452,6 +438,7 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u
}

func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.User, flowType models.FlowType) error {
var err error
config := a.config
otpLength := config.Mailer.OtpLength

Expand All @@ -462,11 +449,8 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U
}

oldToken := u.RecoveryToken
otp, err := crypto.GenerateOtp(otpLength)
if err != nil {
// OTP generation must succeed
panic(err)
}
otp := crypto.GenerateOtp(otpLength)

token := crypto.GenerateTokenHash(u.GetEmail(), otp)
u.RecoveryToken = addFlowPrefixToToken(token, flowType)

Expand Down Expand Up @@ -501,22 +485,16 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models
return err
}

otpNew, err := crypto.GenerateOtp(otpLength)
if err != nil {
// OTP generation must succeed
panic(err)
}
otpNew := crypto.GenerateOtp(otpLength)

u.EmailChange = email
token := crypto.GenerateTokenHash(u.EmailChange, otpNew)
u.EmailChangeTokenNew = addFlowPrefixToToken(token, flowType)

otpCurrent := ""
if config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" {
otpCurrent, err = crypto.GenerateOtp(otpLength)
if err != nil {
// OTP generation must succeed
panic(err)
}
otpCurrent = crypto.GenerateOtp(otpLength)

currentToken := crypto.GenerateTokenHash(u.GetEmail(), otpCurrent)
u.EmailChangeTokenCurrent = addFlowPrefixToToken(currentToken, flowType)
}
Expand Down
7 changes: 3 additions & 4 deletions internal/api/mfa.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,10 +393,9 @@ func (a *API) challengePhoneFactor(w http.ResponseWriter, r *http.Request) error
return tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, generateFrequencyLimitErrorMessage(factor.LastChallengedAt, config.MFA.Phone.MaxFrequency))
}
}
otp, err := crypto.GenerateOtp(config.MFA.Phone.OtpLength)
if err != nil {
panic(err)
}

otp := crypto.GenerateOtp(config.MFA.Phone.OtpLength)

challenge, err := factor.CreatePhoneChallenge(ipAddress, otp, config.Security.DBEncryption.Encrypt, config.Security.DBEncryption.EncryptionKeyID, config.Security.DBEncryption.EncryptionKey)
if err != nil {
return internalServerError("error creating SMS Challenge")
Expand Down
2 changes: 1 addition & 1 deletion internal/api/mfa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ func (ts *MFATestSuite) TestMFAVerifyFactor() {
} else if v.factorType == models.Phone {
friendlyName := uuid.Must(uuid.NewV4()).String()
numDigits := 10
otp, err := crypto.GenerateOtp(numDigits)
otp := crypto.GenerateOtp(numDigits)
require.NoError(ts.T(), err)
phone := fmt.Sprintf("+%s", otp)
f = models.NewPhoneFactor(ts.TestUser, phone, friendlyName)
Expand Down
7 changes: 2 additions & 5 deletions internal/api/phone.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use
now := time.Now()

var otp, messageID string
var err error

if testOTP, ok := config.Sms.GetTestOTP(phone, now); ok {
otp = testOTP
Expand All @@ -93,10 +92,8 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use
return "", tooManyRequestsError(ErrorCodeOverSMSSendRateLimit, "SMS rate limit exceeded")
}
}
otp, err = crypto.GenerateOtp(config.Sms.OtpLength)
if err != nil {
return "", internalServerError("error generating otp").WithInternalError(err)
}
otp = crypto.GenerateOtp(config.Sms.OtpLength)

if config.Hook.SendSMS.Enabled {
input := hooks.SendSMSInput{
User: user,
Expand Down
Loading
Loading