Skip to content
Open
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
2 changes: 1 addition & 1 deletion api/health.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type HealthAPI struct {
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eternal-flame-AD sorry this is kinda big, but would you mind having a look at this? Don't worry about declining if you don't want to review this, or don't have the time. I don't want to burden you with this :D.

Anyway, my plan is to create a snapshot docker build in the coming days and then let the people in #433 try out the functionality to improve the user-guide and find possible bugs with special IdP servers. So there is no rush reviewing this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can look at this tonight first to see if there are large architectural/security changes needed and we can do a final round for final check and nitpicks after the code settles. Probably won't review Android though because I'm not very familar with android dev.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thanks!


// Health returns health information.
// swagger:operation GET /health health getHealth
// swagger:operation GET /health info getHealth
//
// Get health information.
//
Expand Down
362 changes: 362 additions & 0 deletions api/oidc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,362 @@
package api

import (
"context"
"crypto/rand"
"encoding/hex"
"errors"
"fmt"
"log"
"net/http"
"strings"
"sync"
"time"

"github.com/gin-gonic/gin"
"github.com/gotify/server/v2/auth"
"github.com/gotify/server/v2/config"
"github.com/gotify/server/v2/database"
"github.com/gotify/server/v2/model"
"github.com/zitadel/oidc/v3/pkg/client/rp"
httphelper "github.com/zitadel/oidc/v3/pkg/http"
"github.com/zitadel/oidc/v3/pkg/oidc"
)

func NewOIDC(conf *config.Configuration, db *database.GormDatabase, userChangeNotifier *UserChangeNotifier) *OIDCAPI {
scopes := conf.OIDC.Scopes
if len(scopes) == 0 {
scopes = []string{"openid", "profile", "email"}
}

cookieKey := make([]byte, 32)
if _, err := rand.Read(cookieKey); err != nil {
log.Fatalf("failed to generate OIDC cookie key: %v", err)
}
cookieHandlerOpt := []httphelper.CookieHandlerOpt{}
if !conf.Server.SecureCookie {
cookieHandlerOpt = append(cookieHandlerOpt, httphelper.WithUnsecure())
}
cookieHandler := httphelper.NewCookieHandler(cookieKey, cookieKey, cookieHandlerOpt...)

opts := []rp.Option{rp.WithCookieHandler(cookieHandler), rp.WithPKCE(cookieHandler)}

provider, err := rp.NewRelyingPartyOIDC(
context.Background(),
conf.OIDC.Issuer,
conf.OIDC.ClientID,
conf.OIDC.ClientSecret,
conf.OIDC.RedirectURL,
scopes,
opts...,
)
if err != nil {
log.Fatalf("failed to initialize OIDC provider: %v", err)
}

return &OIDCAPI{
DB: db,
Provider: provider,
UserChangeNotifier: userChangeNotifier,
UsernameClaim: conf.OIDC.UsernameClaim,
PasswordStrength: conf.PassStrength,
SecureCookie: conf.Server.SecureCookie,
AutoRegister: conf.OIDC.AutoRegister,
pendingSessions: make(map[string]*pendingOIDCSession),
}
}

const pendingSessionMaxAge = 10 * time.Minute

type pendingOIDCSession struct {
RedirectURI string
ClientName string
CreatedAt time.Time
}

// OIDCAPI provides handlers for OIDC authentication.
type OIDCAPI struct {
DB *database.GormDatabase
Provider rp.RelyingParty
UserChangeNotifier *UserChangeNotifier
UsernameClaim string
PasswordStrength int
SecureCookie bool
AutoRegister bool
pendingSessions map[string]*pendingOIDCSession
pendingSessionsMu sync.Mutex
}

func (a *OIDCAPI) storePendingSession(state string, session *pendingOIDCSession) {
a.pendingSessionsMu.Lock()
defer a.pendingSessionsMu.Unlock()
for s, sess := range a.pendingSessions {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this could be done better, this has O(n^2) complexity over number of users who are authenticating at the same time.

If we are unconditionally iterating through the entire map as the first action of the flow anyways, might as well just use an array (with swap-delete).

A more complex solution is bulk cleanup like this: https://gist.github.com/eternal-flame-AD/e4a47a8a7e6b26bba365a7d24ab84221

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My plan was to have a simple implementation as we probably don't have many parallel logins anyway.

Your decay map doesn't seem to complex, I'll use it, as it's not that much code overhead.

if time.Since(sess.CreatedAt) > pendingSessionMaxAge {
delete(a.pendingSessions, s)
}
}
a.pendingSessions[state] = session
}

func (a *OIDCAPI) popPendingSession(state string) (*pendingOIDCSession, bool) {
a.pendingSessionsMu.Lock()
session, ok := a.pendingSessions[state]
if ok {
delete(a.pendingSessions, state)
}
a.pendingSessionsMu.Unlock()
if !ok || time.Since(session.CreatedAt) > pendingSessionMaxAge {
return nil, false
}
return session, true
}

// swagger:operation GET /auth/oidc/login oidc oidcLogin
//
// Start the OIDC login flow (browser).
//
// Redirects the user to the OIDC provider's authorization endpoint.
// After authentication, the provider redirects back to the callback endpoint.
//
// ---
// parameters:
// - name: name
// in: query
// description: the client name to create after login
// required: true
// type: string
// responses:
// 302:
// description: Redirect to OIDC provider
// default:
// description: Error
// schema:
// $ref: "#/definitions/Error"
func (a *OIDCAPI) LoginHandler() gin.HandlerFunc {
return gin.WrapF(func(w http.ResponseWriter, r *http.Request) {
clientName := r.URL.Query().Get("name")
if clientName == "" {
http.Error(w, "invalid client name", http.StatusBadRequest)
return
}
state, err := a.generateState(clientName)
if err != nil {
http.Error(w, fmt.Sprintf("failed to generate state: %v", err), http.StatusInternalServerError)
return
}
rp.AuthURLHandler(func() string { return state }, a.Provider)(w, r)
})
}

// swagger:operation GET /auth/oidc/callback oidc oidcCallback
//
// Handle the OIDC provider callback (browser).
//
// Exchanges the authorization code for tokens, resolves the user,
// creates a gotify client, sets a session cookie, and redirects to the UI.
//
// ---
// parameters:
// - name: code
// in: query
// description: the authorization code from the OIDC provider
// required: true
// type: string
// - name: state
// in: query
// description: the state parameter for CSRF protection
// required: true
// type: string
// responses:
// 307:
// description: Redirect to UI
// default:
// description: Error
// schema:
// $ref: "#/definitions/Error"
func (a *OIDCAPI) CallbackHandler() gin.HandlerFunc {
callback := func(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens[*oidc.IDTokenClaims], state string, provider rp.RelyingParty, info *oidc.UserInfo) {
user, status, err := a.resolveUser(info)
if err != nil {
http.Error(w, err.Error(), status)
return
}
clientName, _, _ := strings.Cut(state, ":")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the name itself already have a colon in it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I'll reverse the order of the random nonce and the client name and then split by the first colon as the nonce is hex encoded and never includes a colon.

client, err := a.createClient(clientName, user.ID)
if err != nil {
http.Error(w, fmt.Sprintf("failed to create client: %v", err), http.StatusInternalServerError)
return
}
auth.SetCookie(w, client.Token, auth.CookieMaxAge, a.SecureCookie)
// A reverse proxy may have already stripped a url prefix from the URL
// without us knowing, we have to make a relative redirect.
// We cannot use http.Redirect as this normalizes the Path with r.URL.
w.Header().Set("Location", "../../")
w.WriteHeader(http.StatusTemporaryRedirect)
}
return gin.WrapF(rp.CodeExchangeHandler(rp.UserinfoCallback(callback), a.Provider))
}

// swagger:operation POST /auth/oidc/external/authorize oidc externalAuthorize
//
// Initiate the OIDC authorization flow for a native app.
//
// The app generates a PKCE code_verifier and code_challenge, then calls this
// endpoint. The server forwards the code_challenge to the OIDC provider and
// returns the authorization URL for the app to open in a browser.
//
// ---
// consumes: [application/json]
// produces: [application/json]
// parameters:
// - name: body
// in: body
// required: true
// schema:
// $ref: "#/definitions/OIDCExternalAuthorizeRequest"
// responses:
// 200:
// description: Ok
// schema:
// $ref: "#/definitions/OIDCExternalAuthorizeResponse"
// default:
// description: Error
// schema:
// $ref: "#/definitions/Error"
func (a *OIDCAPI) ExternalAuthorizeHandler(ctx *gin.Context) {
var req model.OIDCExternalAuthorizeRequest
if err := ctx.ShouldBindJSON(&req); err != nil {
ctx.AbortWithError(http.StatusBadRequest, err)
return
}
state, err := a.generateState(req.Name)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this how it works for VaultWarden (server generates state) ? I'm not super sure what this is for. From my cursory exploration it seems like in VaultWarden state is simply for the client's convenience (might be wrong..)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay this seems to be documented in RFC6749, I got it messed up. The flow is right although the bookkeeping ClientName could be clearer (don't keep them both in the server memory and in the token text at the same time).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is different from vaultwarden, but as I understood is this for CSRF protection, so basically the android app has to check the initial state against the state that will be received on the redirect uri. So I don't think it matters.

Do you mean removing the client name from the pending session struct and then directly reading it from the state similar to how it's done in the web ui flow?

if err != nil {
ctx.AbortWithError(http.StatusInternalServerError, err)
return
}
a.storePendingSession(state, &pendingOIDCSession{
RedirectURI: req.RedirectURI, ClientName: req.Name, CreatedAt: time.Now(),
})
authOpts := []rp.AuthURLOpt{
rp.AuthURLOpt(rp.WithURLParam("redirect_uri", req.RedirectURI)),
rp.WithCodeChallenge(req.CodeChallenge),
}
ctx.JSON(http.StatusOK, &model.OIDCExternalAuthorizeResponse{
AuthorizeURL: rp.AuthURL(state, a.Provider, authOpts...),
State: state,
})
}

// swagger:operation POST /auth/oidc/external/token oidc externalToken
//
// Exchange an authorization code for a gotify client token.
//
// After the user authenticates with the OIDC provider and the app receives
// the authorization code via redirect, the app calls this endpoint with the
// code and PKCE code_verifier. The server exchanges the code with the OIDC
// provider and returns a gotify client token.
//
// ---
// consumes: [application/json]
// produces: [application/json]
// parameters:
// - name: body
// in: body
// required: true
// schema:
// $ref: "#/definitions/OIDCExternalTokenRequest"
// responses:
// 200:
// description: Ok
// schema:
// $ref: "#/definitions/OIDCExternalTokenResponse"
// default:
// description: Error
// schema:
// $ref: "#/definitions/Error"
func (a *OIDCAPI) ExternalTokenHandler(ctx *gin.Context) {
var req model.OIDCExternalTokenRequest
if err := ctx.ShouldBindJSON(&req); err != nil {
ctx.AbortWithError(http.StatusBadRequest, err)
return
}
session, ok := a.popPendingSession(req.State)
if !ok {
ctx.AbortWithError(http.StatusBadRequest, errors.New("unknown or expired state"))
return
}
exchangeOpts := []rp.CodeExchangeOpt{
rp.CodeExchangeOpt(rp.WithURLParam("redirect_uri", session.RedirectURI)),
rp.WithCodeVerifier(req.CodeVerifier),
}
tokens, err := rp.CodeExchange[*oidc.IDTokenClaims](ctx.Request.Context(), req.Code, a.Provider, exchangeOpts...)
if err != nil {
ctx.AbortWithError(http.StatusUnauthorized, fmt.Errorf("token exchange failed: %w", err))
return
}
info, err := rp.Userinfo[*oidc.UserInfo](ctx.Request.Context(), tokens.AccessToken, tokens.TokenType, tokens.IDTokenClaims.GetSubject(), a.Provider)
if err != nil {
ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to get user info: %w", err))
return
}
user, status, resolveErr := a.resolveUser(info)
if resolveErr != nil {
ctx.AbortWithError(status, resolveErr)
return
}
client, err := a.createClient(session.ClientName, user.ID)
if err != nil {
ctx.AbortWithError(http.StatusInternalServerError, err)
return
}
ctx.JSON(http.StatusOK, &model.OIDCExternalTokenResponse{
Token: client.Token,
User: &model.UserExternal{ID: user.ID, Name: user.Name, Admin: user.Admin},
})
}

func (a *OIDCAPI) generateState(name string) (string, error) {
nonce := make([]byte, 20)
if _, err := rand.Read(nonce); err != nil {
return "", err
}
return name + ":" + hex.EncodeToString(nonce), nil
}

// resolveUser looks up or creates a user from OIDC userinfo claims.
func (a *OIDCAPI) resolveUser(info *oidc.UserInfo) (*model.User, int, error) {
usernameRaw, ok := info.Claims[a.UsernameClaim]
if !ok {
return nil, http.StatusInternalServerError, fmt.Errorf("username claim %q is missing", a.UsernameClaim)
}
username := fmt.Sprint(usernameRaw)
if username == "" || usernameRaw == nil {
return nil, http.StatusInternalServerError, fmt.Errorf("username claim was empty")
}

user, err := a.DB.GetUserByName(username)
if err != nil {
return nil, http.StatusInternalServerError, fmt.Errorf("database error: %w", err)
}
if user == nil {
if !a.AutoRegister {
return nil, http.StatusForbidden, fmt.Errorf("user does not exist and auto-registration is disabled")
}
user = &model.User{Name: username, Admin: false, Pass: nil}
if err := a.DB.CreateUser(user); err != nil {
return nil, http.StatusInternalServerError, fmt.Errorf("failed to create user: %w", err)
}
if err := a.UserChangeNotifier.fireUserAdded(user.ID); err != nil {
log.Printf("Could not notify user change: %v\n", err)
}
}
return user, 0, nil
}

func (a *OIDCAPI) createClient(name string, userID uint) (*model.Client, error) {
client := &model.Client{
Name: name,
Token: auth.GenerateNotExistingToken(generateClientToken, func(t string) bool { c, _ := a.DB.GetClientByToken(t); return c != nil }),
UserID: userID,
}
return client, a.DB.CreateClient(client)
}
Loading
Loading