Add SFTPGo Authentication and Provisioning Service#71
Open
rksk wants to merge 11 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new SFTPGo Authentication Service with REST endpoints (/prelogin-hook, /auth-hook), IdP and SFTPGo Admin integrations, MySQL-backed session persistence, config/logging/http client utilities, OpenAPI spec, Dockerfile, README/LICENSE, migrations, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant SFTPGo as SFTPGo
participant Handler as PreLoginHook_Handler
participant IdP as IdP_Service
participant Subscription as Subscription_API
participant SFTPAdmin as SFTPGo_Admin_API
participant DB as Database
Client->>SFTPGo: User login attempt
SFTPGo->>Handler: POST /prelogin-hook (username)
Handler->>Handler: Validate API key
Handler->>IdP: SCIM lookup (GetAsgardeoUser)
IdP-->>Handler: User data
Handler->>Subscription: GetUserFolderList(username)
Subscription-->>Handler: Project keys / folders
Handler->>SFTPAdmin: ProvisionFolders / UpdateUser
SFTPAdmin-->>Handler: Provisioning result
Handler->>DB: Optional audit/session save
Handler-->>SFTPGo: MinimalSFTPGoUser (200)
sequenceDiagram
actor Client
participant SFTPGo as SFTPGo
participant Handler as AuthHandler
participant DB as Database
participant IdP as IdP_Service
participant SFTPAdmin as SFTPGo_Admin_API
Client->>SFTPGo: Start keyboard-interactive auth (step 1)
SFTPGo->>Handler: POST /auth-hook (requestId, step, username)
Handler->>Handler: Validate API key
alt Step 1
Handler->>IdP: InitFlow(username)
IdP-->>Handler: FlowId + NextStep
Handler->>DB: SaveSession(requestId, sessionData)
Handler-->>SFTPGo: AuthHookResponse (questions)
else Subsequent step
Handler->>DB: GetSession(requestId)
DB-->>Handler: sessionData
Handler->>IdP: PostToAuthnEndpoint(payload)
IdP-->>Handler: IdPResponse
alt Incomplete
Handler->>DB: UpdateSession
Handler-->>SFTPGo: Next questions
else Success
Handler->>SFTPAdmin: UpdateUser (permissions/folders)
Handler->>DB: DeleteSession
Handler-->>SFTPGo: AuthHookResponse (success)
else Failure
Handler->>DB: DeleteSession
Handler-->>SFTPGo: AuthHookResponse (failure)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Fix all issues with AI agents
In `@operations/sftpgo-authentication-service/.gitignore`:
- Around line 2-3: Remove the accidental stray ignore pattern "*.exe~" from the
.gitignore in the sftpgo-authentication-service; either delete that specific
line or replace it with a generic backup pattern like "*~" if you intend to
ignore editor backup files, ensuring consistency with other entries (do not add
file content examples here).
In `@operations/sftpgo-authentication-service/cmd/server/main.go`:
- Around line 46-49: The InitEmailRegex error is currently logged but startup
continues; update the main initialization to fail fast when
util.InitEmailRegex(cfg.EmailRegexPattern) returns an error by logging the error
and terminating startup (e.g., call logger.Fatalf or logger.Error(...) followed
by os.Exit(1) or return from main). Modify the block around util.InitEmailRegex
to ensure the process exits with a non-zero status on error so validation won't
run with an uninitialized regex.
- Around line 71-74: The http.Server instance named server is missing
ReadTimeout, WriteTimeout, and IdleTimeout to mitigate slowloris-style attacks;
update the server literal where http.Server is constructed (the server variable)
to include ReadTimeout, WriteTimeout, and IdleTimeout with sensible defaults
(e.g., ReadTimeout ~5s, WriteTimeout ~10s, IdleTimeout ~120s) and add the time
package import so these values use time.Duration (time.Second). Keep the
existing Addr and Handler settings unchanged.
In `@operations/sftpgo-authentication-service/go.mod`:
- Line 18: Update the module Go version directive in go.mod from "go 1.24" to
"go 1.25.6" (edit the go directive in the go.mod file), then run go mod tidy and
rebuild/tests to ensure dependencies and toolchain compatibility; also update
any CI/workflow specs or Dockerfiles that pin Go to 1.24 to use 1.25.6 so the
project and CI use the same Go version.
- Around line 20-22: The go.mod currently pins github.com/go-sql-driver/mysql to
v1.8.1 which is outdated; update the MySQL driver to v1.9.3 by running `go get
github.com/go-sql-driver/[email protected]`, then run `go mod tidy` to refresh
go.mod/go.sum (leave filippo.io/edwards25519 at v1.1.0 as-is), and run the test
suite/integration tests to verify there are no regressions in the authentication
code that uses the mysql driver.
In `@operations/sftpgo-authentication-service/internal/config/config_test.go`:
- Around line 65-74: TestLoad_MissingCritical currently calls os.Clearenv() and
doesn't restore the process environment, which can break other tests; modify the
test to snapshot the current environment (e.g., capture os.Environ()), call
os.Clearenv(), then defer a restore that repopulates the environment (or clears
and repopulates from the snapshot) after the test completes; ensure you
reference the TestLoad_MissingCritical test and the Load() call when making
changes and add the "strings" import as noted.
In `@operations/sftpgo-authentication-service/internal/handler/handler.go`:
- Around line 224-275: The audit log in AuthHandler currently logs the raw
project key when isProjectKeyStep is true; change this to redact or hash the
project key before including it in the detail passed to auditLog: locate the
branch that builds detail using req.Answers[0] (in AuthHandler) and replace the
direct value with a sanitized version (e.g., a deterministic hash or masked
value like showing only last N chars) so the raw key is never logged; ensure the
redaction/hashing logic is applied only to the value used in detail and that no
other logs or variables (resp, req) are left logging the raw key.
- Around line 68-220: The PreLoginHook handler currently returns an anonymous
user on IdP lookup failure; remove the anonymous fallback and instead return
http.StatusNotFound to fail closed. In PreLoginHook, delete the
anonymousUsername / home / perms / vfs construction and the writeJSONResponse
call that returns the anonymous user, and replace that block with a single
w.WriteHeader(http.StatusNotFound) (or use http.Error) after calling
h.auditLog(r, u.Username, "pre-login-hook", "denied", "user not found in IdP").
The change affects the error branch that checks err after calling
h.idp.GetAsgardeoUser; ensure the audit log message remains and the function
returns immediately after sending 404.
In `@operations/sftpgo-authentication-service/internal/handler/utils_test.go`:
- Around line 45-57: The test TestSanitizeUsername in utils_test.go uses the
wrong expected value and contains no assertion; update the expected string to
"user_email_com" to match util.SanitizeUsername (which replaces both '@' and '.'
with '_'), and replace the no-op conditional with a proper t.Fatalf or t.Errorf
assertion that fails when sanitizeUsername(input) != expected; reference the
sanitizeUsername wrapper and util.SanitizeUsername behavior when making the
change.
In `@operations/sftpgo-authentication-service/internal/handler/utils.go`:
- Around line 195-205: The handler handleProjectKeyStep currently uses
unvalidated projectKey to build virtualPath and mappedPath which allows path
traversal; validate and sanitize projectKey before use by enforcing an allowlist
(e.g., only lowercase letters, numbers, hyphen/underscore) and rejecting inputs
containing path separators or dots, and after building mappedPath with
filepath.Join(h.cfg.FolderPath, projectKey) verify the final path does not
escape the base folder (use filepath.Clean + filepath.Rel to ensure the relative
path does not start with ".."); fail the request (set resp.AuthResult =
AuthResultFailure and a clear Instruction) if validation or the final
base-folder check fails so UserVirtualFolder entries are never created from
malicious input.
- Around line 78-89: The handler handleAuthStep1 currently uses req.Username
directly when initiating the IdP flow and logging; run the existing
validateUsername(req.Username) first and if it returns false set resp.AuthResult
= AuthResultFailure and resp.Instruction to a clear failure message, then return
without calling h.idp.InitFlow; also avoid logging the raw username in
h.logger.Error — either log a sanitized/masked username or omit it entirely when
reporting the InitFlow error. This ensures CR/LF and oversize usernames are
rejected before reaching idp.InitFlow or being written to logs.
In `@operations/sftpgo-authentication-service/internal/httpclient/client.go`:
- Around line 55-80: The logRequest and logResponse functions currently read up
to 10KB from req.Body/resp.Body and then replace the body with only those bytes,
truncating larger payloads; instead, read the entire body into a buffer (or
stream through a TeeReader) to restore the full body for transmission, but when
logging only use the first 10KB for Trace output. Concretely, in
LoggingTransport.logRequest and LoggingTransport.logResponse capture the full
body (e.g., io.ReadAll or io.Copy into a bytes.Buffer), then set
req.Body/resp.Body back to a new ReadCloser that wraps the full buffer, and pass
a sliced view (first 10KB) to t.Logger.Trace so logs are limited while the full
body is preserved for the actual request/response.
In `@operations/sftpgo-authentication-service/internal/log/logger.go`:
- Around line 107-111: AppLogger.Errorf currently pre-formats the message then
calls AppLogger.Error which treats its input as a format string, causing
double-formatting; fix Errorf to send the already-formatted message to the
underlying logger using a safe "%s" format (or a non-formatting Print/Println)
instead of passing the raw formatted string as a format specifier, e.g. call the
logger with "%s", msg (or logger.Print/Println) and return errors.New(msg);
update the AppLogger.Errorf implementation and ensure you reference
AppLogger.Errorf and the underlying logger used by AppLogger.Error when making
the change.
In `@operations/sftpgo-authentication-service/internal/models/models.go`:
- Around line 81-96: The JSON tags on the KeyIntRequest struct are camelCase but
must match the OpenAPI snake_case names; update the struct tags on KeyIntRequest
(fields RequestID, Step, Username, Answers) to use "request_id", "step",
"username", and "answers" respectively so JSON unmarshalling aligns with the
OpenAPI schema and incoming SFTPGo payloads.
In `@operations/sftpgo-authentication-service/internal/service/idp.go`:
- Around line 140-182: The InitFlow method on IdPService is including the client
secret in the authorize request form (form.Set("client_secret",
ctx.clientSecret)), which must be removed; update InitFlow to stop adding
ctx.clientSecret to the form (leave client_secret out of the authorize POST) so
only client_id, response_type, redirect_uri, scope and response_mode are sent to
ctx.authorizeEP, and keep all token-exchange uses of the secret in
getBearerToken (or other server-to-server token endpoint code) unchanged.
In `@operations/sftpgo-authentication-service/internal/service/sftpgo.go`:
- Around line 101-129: ProvisionFolders currently logs validation/check/create
failures but always returns nil; modify ProvisionFolders (method on
SFTPGoService) to propagate errors to callers: when validateFolderName,
checkFolderExists, or createFolder return an error, either return that error
immediately (fail-fast) or append it to an aggregated error slice and return a
combined error at the end; ensure getAdminToken errors are already propagated
and that any returned error includes context (folder name and operation) so
callers can react appropriately.
In `@operations/sftpgo-authentication-service/openapi.yaml`:
- Around line 237-243: The OpenAPI spec currently defines the ApiKeyAuth
security scheme under components.securitySchemes but lacks a top-level servers
declaration to enforce HTTPS; add a top-level servers block (e.g., servers: -
url: "https://api.example.com" description: "HTTPS API endpoint") at the root of
openapi.yaml so all endpoints using ApiKeyAuth require TLS, ensuring API keys
are not sent over cleartext (update the root document near components and
reference ApiKeyAuth implicitly via this servers entry).
In `@operations/sftpgo-authentication-service/README.md`:
- Line 68: The README's Go version "Go 1.23+" is inconsistent with the
Dockerfile base image `golang:1.24-alpine`; update the README to match the
Dockerfile (change "Go 1.23+" to "Go 1.24+") or, if you prefer to stay on 1.23,
update the Dockerfile image tag accordingly—ensure the version string in
README.md and the Dockerfile image tag (`golang:1.24-alpine`) are identical so
documentation and build config align.
🧹 Nitpick comments (13)
operations/sftpgo-authentication-service/go.mod (1)
20-22: Surprisingly minimal dependency list for the described service scope.The PR describes a comprehensive service with HTTP handlers, advanced logging, OpenAPI specs, IdP integration, and resilient HTTP clients. However, only the MySQL driver is declared as a direct dependency. While Go's standard library is powerful, confirm that:
- No HTTP routing framework (e.g., chi, gorilla/mux, gin) is needed
- Standard library logging meets "advanced logging and audit" requirements
- No structured logging library (e.g., zap, zerolog) is required
- OpenAPI spec generation/validation doesn't require additional tooling
- HTTP client resilience patterns are implemented without helper libraries
If the standard library approach is intentional, this is commendable for minimizing dependencies. However, if additional libraries would improve maintainability or reduce custom boilerplate (especially for structured logging and HTTP client resilience), consider evaluating them.
operations/sftpgo-authentication-service/Dockerfile (2)
33-33: Pin Alpine version and install CA certificates.Using
alpine:latestcan cause non-reproducible builds. Since this service makes HTTPS calls to Asgardeo and external APIs,ca-certificatesis required in the final image.🔧 Proposed fix
-FROM alpine:latest +FROM alpine:3.19 # Set the working directory to /app WORKDIR /app + +# Install CA certificates for HTTPS connections +RUN apk --no-cache add ca-certificates
23-27: Consider optimizing Docker layer caching.Copying
go.modandgo.sumfirst and runninggo mod downloadbefore copying the full source improves build cache efficiency when only source files change.♻️ Suggested optimization
-# Copy the Go source file into the container -COPY . . +# Copy go.mod and go.sum first for better caching +COPY go.mod go.sum ./ +RUN go mod download -# Run unit tests -RUN go test ./... +# Copy the rest of the source +COPY . . +# Run unit tests +RUN go test ./...operations/sftpgo-authentication-service/.env.example (2)
28-28: Inline comments may not parse correctly in all .env loaders.Some
.envparsers don't support inline comments. Consider moving the comment to a separate line above.♻️ Suggested fix
-CHECK_ROLE="internal" # The role display name to check for internal users +# The role display name to check for internal users +CHECK_ROLE="internal"
36-36: Add trailing newline at end of file.Missing newline at EOF per POSIX conventions and linter warning.
♻️ Suggested fix
DB_CONN_STRING="user:password@tcp(127.0.0.1:3306)/sftpgo_sessions" +operations/sftpgo-authentication-service/internal/util/util.go (2)
52-58: SimplifyIsLikelyEmailand consider nil guard.The function can be simplified. Also, if
emailRegexis somehow nil (e.g., custom pattern fails during startup before proper error handling), this would panic.♻️ Suggested simplification
// IsLikelyEmail checks if a string broadly resembles an email address. func IsLikelyEmail(s string) bool { - if !emailRegex.MatchString(s) { - return false - } - return true + return emailRegex != nil && emailRegex.MatchString(s) }
42-45: Consider logging or returning error from init.The
init()function silently discards the error fromInitEmailRegex. If the default pattern ever becomes invalid, this would be a silent failure causingIsLikelyEmailto potentially panic or behave unexpectedly.♻️ Suggested improvement
func init() { // Initialize with default pattern - _ = InitEmailRegex("") + if err := InitEmailRegex(""); err != nil { + panic("failed to compile default email regex: " + err.Error()) + } }operations/sftpgo-authentication-service/internal/util/init_test.go (2)
41-49: Hardcoded pattern creates maintenance burden.
TestEmailRegexCompilationduplicates the pattern string fromutil.go. If the default pattern changes, this test won't verify the actual pattern used by the package.Consider either:
- Exporting the default pattern for testing, or
- Removing this test since
TestInitEmailRegexalready verifies compilation works.
24-39: Consider adding negative test cases.The test only validates that a valid email passes. Adding cases for invalid emails (e.g., missing
@, invalid TLD, consecutive dots) would strengthen the test coverage.♻️ Suggested addition
// Add after line 38 // Test invalid emails should not match invalidEmails := []string{ "plaintext", "@example.com", "user@", "[email protected]", "user@@example.com", } for _, email := range invalidEmails { if IsLikelyEmail(email) { t.Errorf("Default regex should NOT match invalid email: %s", email) } }operations/sftpgo-authentication-service/openapi.yaml (1)
336-341: Add payload bounds to user-supplied arrays.
ConsidermaxItems(and item length limits) foranswers(and similarly forquestions/echos) to reduce oversized payload risk when using OpenAPI-based validation/gateways.♻️ Example (pick appropriate limits)
answers: type: array description: An array of strings containing the user's responses to questions from the previous step. items: type: string + maxLength: 1024 + maxItems: 4operations/sftpgo-authentication-service/internal/config/config.go (1)
162-177: Normalize base paths before concatenating endpoints.
If base paths include a trailing/, the current concatenation yields//in computed endpoints. Trimming once improves robustness.♻️ Suggested fix
- // Compute endpoints - cfg.AdminTokenEP = cfg.SFTPGoBasePath + "/token" - cfg.SftpgoFoldersEP = cfg.SFTPGoBasePath + "/folders" - cfg.SftpgoUsersEP = cfg.SFTPGoBasePath + "/users" - cfg.IdPTokenEP = cfg.InternalIdPBasePath + "/oauth2/token" - cfg.IdPSCIMUsersEP = cfg.InternalIdPBasePath + "/scim2/Users" - cfg.IdPAuthnEP = cfg.InternalIdPBasePath + "/oauth2/authn" - cfg.IdPAuthorizeEP = cfg.InternalIdPBasePath + "/oauth2/authorize/" + // Compute endpoints + sftpgoBase := strings.TrimRight(cfg.SFTPGoBasePath, "/") + idpBase := strings.TrimRight(cfg.InternalIdPBasePath, "/") + cfg.AdminTokenEP = sftpgoBase + "/token" + cfg.SftpgoFoldersEP = sftpgoBase + "/folders" + cfg.SftpgoUsersEP = sftpgoBase + "/users" + cfg.IdPTokenEP = idpBase + "/oauth2/token" + cfg.IdPSCIMUsersEP = idpBase + "/scim2/Users" + cfg.IdPAuthnEP = idpBase + "/oauth2/authn" + cfg.IdPAuthorizeEP = idpBase + "/oauth2/authorize/" @@ - if cfg.ExternalIdPBasePath != "" { - cfg.ExternalIdPTokenEP = cfg.ExternalIdPBasePath + "/oauth2/token" - cfg.ExternalIdPSCIMUsersEP = cfg.ExternalIdPBasePath + "/scim2/Users" - cfg.ExternalIdPAuthnEP = cfg.ExternalIdPBasePath + "/oauth2/authn" - cfg.ExternalIdPAuthorizeEP = cfg.ExternalIdPBasePath + "/oauth2/authorize/" - } + if cfg.ExternalIdPBasePath != "" { + externalBase := strings.TrimRight(cfg.ExternalIdPBasePath, "/") + cfg.ExternalIdPTokenEP = externalBase + "/oauth2/token" + cfg.ExternalIdPSCIMUsersEP = externalBase + "/scim2/Users" + cfg.ExternalIdPAuthnEP = externalBase + "/oauth2/authn" + cfg.ExternalIdPAuthorizeEP = externalBase + "/oauth2/authorize/" + }operations/sftpgo-authentication-service/internal/service/sftpgo.go (1)
52-85: Validate and URL-escape identifiers before building path segments.
usernameandprojectKeyare interpolated into URLs directly; escaping and reusingvalidateFolderNamewill harden against invalid characters and path-segment issues.🔒 Suggested hardening
import ( "bytes" "encoding/json" "fmt" "io" "net/http" + "net/url" "path/filepath" "strings" "time" @@ func (s *SFTPGoService) UpdateUser(username, projectKey string, perms map[string][]string, vfs []models.UserVirtualFolder) error { s.logger.Debug("Updating user %s to provide access to %s", username, projectKey) + if err := validateFolderName(projectKey); err != nil { + return s.logger.Errorf("invalid project key '%s': %v", projectKey, err) + } @@ - sftpgoUserEP := s.cfg.SftpgoUsersEP + "/" + username + sftpgoUserEP := s.cfg.SftpgoUsersEP + "/" + url.PathEscape(username) @@ func (s *SFTPGoService) checkFolderExists(name, token string) (bool, error) { - endpoint := s.cfg.SftpgoFoldersEP + "/" + name + endpoint := s.cfg.SftpgoFoldersEP + "/" + url.PathEscape(name)Also applies to: 156-167
operations/sftpgo-authentication-service/internal/service/subscription.go (1)
41-48: Use configured HTTP timeout instead of a fixed 10s.
Keeps behavior consistent with other services and honors config.♻️ Suggested change
func NewSubscriptionService(cfg *config.Config, logger *log.AppLogger) *SubscriptionService { return &SubscriptionService{ cfg: cfg, logger: logger, - client: httpclient.NewLoggingClient(10*time.Second, logger), + client: httpclient.NewLoggingClient(time.Duration(cfg.HTTPTimeout)*time.Second, logger), } }
rksk
force-pushed
the
sftp-server-webooks
branch
from
9efa03b to
12bf920
Compare
rksk
force-pushed
the
sftp-server-webooks
branch
from
12bf920 to
5436778
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@operations/sftpgo-authentication-service/.env.example`:
- Around line 35-36: The comment header is merged with its text and the
DB_CONN_STRING line lacks a trailing newline; separate and space the comment as
e.g. "# Database Connection (MySQL) Format: user:password@tcp(host:port)/dbname"
(or similar readable spacing) and ensure the
DB_CONN_STRING="user:password@tcp(127.0.0.1:3306)/sftpgo_sessions" line ends
with a newline (add a final line break at EOF) so tools that require trailing
newlines and clearer comments will work; reference DB_CONN_STRING to locate the
value to update and the comment above it.
In `@operations/sftpgo-authentication-service/internal/handler/handler.go`:
- Around line 185-187: The provisioning error is currently logged via
h.logger.Error after calling h.sftpgo.ProvisionFolders(folders) but the handler
continues and returns 200, which is inconsistent with the empty-folder 204 path;
change the handler so that if h.sftpgo.ProvisionFolders(folders) returns an
error you: (1) create an audit/error entry (use the same audit mechanism used
elsewhere), (2) log the error with context including u.Username and the error,
and (3) abort processing by returning a 500 response to the caller instead of
continuing to return 200; update the code around the h.sftpgo.ProvisionFolders
call (and remove the current non-fatal h.logger.Error-only behavior) so
provisioning failures are treated as fatal.
In `@operations/sftpgo-authentication-service/internal/httpclient/client.go`:
- Around line 55-74: The logRequest method on LoggingTransport currently uses
io.ReadAll on req.Body which can OOM for large payloads; change logRequest (and
the analogous trace logging in logResponse) to read at most 10KB for tracing by
using a LimitedReader or io.ReadFull into a fixed-size buffer, capture the
prefix for logging, then reconstruct req.Body (and resp.Body in the response
path) by concatenating the prefix buffer with the remaining unread stream so the
full body is preserved for transmission/consumption; ensure you handle nil
bodies and reset the Body to io.NopCloser over a combined reader after reading
the prefix.
In `@operations/sftpgo-authentication-service/internal/service/sftpgo.go`:
- Around line 60-63: In UpdateUser, separate the error path from the existence
boolean returned by checkFolderExists: call exists, err :=
s.checkFolderExists(projectKey, token) then if err != nil return
s.logger.Errorf(...) with a message like "failed to check folder '%s' before
user update: %v" including the err, and only if err == nil then if !exists
return s.logger.Errorf("folder '%s' does not exist, cannot update user"); mirror
the pattern used in ProvisionFolders to avoid masking transport/auth/status
errors when calling checkFolderExists.
🧹 Nitpick comments (20)
operations/sftpgo-authentication-service/Dockerfile (1)
33-33: Consider pinning the Alpine version for reproducible builds.Using
alpine:latestcan lead to non-reproducible builds and unexpected behavior when the base image is updated.♻️ Proposed fix
-FROM alpine:latest +FROM alpine:3.19operations/sftpgo-authentication-service/.env.example (1)
26-28: Clarify the distinction between FOLDER_PATH and DIR_PATH.Both variables have the same placeholder value (
/path/on/sftpgo/server), making it unclear what each is used for. Consider adding comments explaining their distinct purposes.📝 Proposed improvement
`#SFTPGo` User/Folder Configuration -FOLDER_PATH="/path/on/sftpgo/server" -DIR_PATH="/path/on/sftpgo/server" +FOLDER_PATH="/path/on/sftpgo/server" # Base path for virtual folders +DIR_PATH="/path/on/sftpgo/server" # Base path for user home directories CHECK_ROLE="internal" # The role display name to check for internal usersoperations/sftpgo-authentication-service/README.md (2)
23-37: Add language specifier to fenced code block.The architecture diagram code block lacks a language specifier. Use
textorplaintextfor ASCII diagrams.📝 Proposed fix
-``` +```text ┌─────────────┐ ┌──────────────────┐ ┌─────────────────┐
41-62: Add language specifier to project structure code block.📝 Proposed fix
-``` +```text . ├── cmd/server/main.go # Application entry pointoperations/sftpgo-authentication-service/internal/util/util.go (3)
42-45: Silently ignoring init error could mask issues.While the default pattern is unlikely to fail compilation, ignoring the error silently makes debugging harder if a custom pattern with issues is somehow introduced.
📝 Proposed improvement
func init() { // Initialize with default pattern - _ = InitEmailRegex("") + if err := InitEmailRegex(""); err != nil { + panic("failed to compile default email regex: " + err.Error()) + } }
52-58: Simplify IsLikelyEmail return statement.The conditional can be reduced to a single return.
♻️ Proposed simplification
// IsLikelyEmail checks if a string broadly resembles an email address. func IsLikelyEmail(s string) bool { - if !emailRegex.MatchString(s) { - return false - } - return true + return emailRegex.MatchString(s) }
33-40: Thread-safety concern if InitEmailRegex is called concurrently.
emailRegexis a package-level variable modified without synchronization. IfInitEmailRegexis called from multiple goroutines, there's a data race. This is likely acceptable since initialization typically happens once at startup, but worth noting.operations/sftpgo-authentication-service/internal/util/init_test.go (1)
41-49: Pattern duplication creates maintenance risk.The pattern on line 43 is duplicated from
util.go. If the default pattern changes inutil.go, this test won't detect compilation issues with the actual pattern used.Consider testing compilation through the exported
InitEmailRegexfunction instead.♻️ Proposed improvement
func TestEmailRegexCompilation(t *testing.T) { - // Test that the refined simple default pattern compiles - pattern := `^[\p{L}0-9!#$'%*+=?^_{|}~&-]+(?:\.[\p{L}0-9!#$'%*+=?^_{|}~&-]+)*@[\p{L}0-9.\-_]+\.[a-zA-Z]{2,10}$` - - _, err := regexp.Compile(pattern) + // Test that the default pattern compiles correctly + err := InitEmailRegex("") if err != nil { t.Fatalf("Default pattern failed to compile: %v", err) } }operations/sftpgo-authentication-service/internal/service/idp_test.go (1)
62-75: Verify case-sensitivity expectations for internal-user matching.
Email domains are generally case-insensitive; the test enforces a strict case match (e.g.,[email protected]→ false). If internal routing should be case-insensitive, normalize input (e.g., strings.ToLower) and update this test accordingly.operations/sftpgo-authentication-service/internal/service/subscription_test.go (1)
70-98: Consider adding error handling tests.The happy path tests are solid. For improved robustness, consider adding tests for error scenarios such as server timeouts, malformed JSON responses, or network failures. This would help ensure the service degrades gracefully.
operations/sftpgo-authentication-service/cmd/server/main.go (2)
71-77: AddReadHeaderTimeoutfor complete slowloris mitigation.While
ReadTimeout,WriteTimeout, andIdleTimeoutare now configured (addressing previous feedback),ReadHeaderTimeoutis still missing. This timeout specifically limits how long the server waits for request headers and is the primary defense against slowloris attacks where attackers send headers very slowly.🛡️ Suggested improvement
server := &http.Server{ Addr: ":" + cfg.Port, Handler: mux, + ReadHeaderTimeout: 5 * time.Second, ReadTimeout: time.Duration(cfg.ReadTimeout) * time.Second, WriteTimeout: time.Duration(cfg.WriteTimeout) * time.Second, IdleTimeout: time.Duration(cfg.IdleTimeout) * time.Second, }
65-68: Consider adding a health check endpoint.For operational readiness in Choreo/Kubernetes environments, a
/healthor/readyendpoint is typically beneficial for liveness and readiness probes.💡 Example
mux.HandleFunc("/health", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) w.Write([]byte("OK")) })operations/sftpgo-authentication-service/internal/config/config.go (2)
86-92:IsSensitivefield is unused.The
EnvVar.IsSensitivefield is defined but never utilized invalidateEnvVars. If this is intended for future log masking or audit purposes, consider adding a TODO comment. Otherwise, it can be removed to reduce confusion.
193-208: Normalize base paths to prevent double-slash issues in computed endpoints.If environment variables like
SFTPGO_API_BASEorINTERNAL_IDP_BASE_PATHinclude trailing slashes, the computed endpoints will contain double slashes (e.g.,https://api.example.com//oauth2/token), which may cause request failures depending on the backend server configuration.🔧 Suggested fix
+ // Normalize base paths by removing trailing slashes + cfg.SFTPGoBasePath = strings.TrimRight(cfg.SFTPGoBasePath, "/") + cfg.InternalIdPBasePath = strings.TrimRight(cfg.InternalIdPBasePath, "/") + if cfg.ExternalIdPBasePath != "" { + cfg.ExternalIdPBasePath = strings.TrimRight(cfg.ExternalIdPBasePath, "/") + } + // Compute endpoints cfg.AdminTokenEP = cfg.SFTPGoBasePath + "/token"Add this normalization block before computing endpoints (around line 193).
operations/sftpgo-authentication-service/internal/service/sftpgo.go (1)
220-226: Use a clearer error for empty folder names.
io.ErrShortBufferis misleading here; a dedicated error improves intent and debugging.operations/sftpgo-authentication-service/internal/config/config_test.go (1)
25-45: Avoid process-wide env leakage across tests.
defer os.Clearenv()wipes the process env for subsequent tests. Consider snapshot+restore or per-test env helpers so other tests don’t inherit an empty env.Also applies to: 90-112
operations/sftpgo-authentication-service/openapi.yaml (1)
299-304: AddmaxItemsto array fields to cap payload size.Arrays like permissions items, answers, questions, and echos are unbounded; consider adding explicit
maxItemslimits to mitigate abuse. (CKV_OPENAPI_21)Also applies to: 339-344, 359-369
operations/sftpgo-authentication-service/internal/service/subscription.go (1)
41-47: Use configured HTTP timeout for subscription calls.The client timeout is hard-coded; reuse
cfg.HTTPTimeoutto align with the rest of the service and configuration.♻️ Suggested change
- client: httpclient.NewLoggingClient(10*time.Second, logger), + client: httpclient.NewLoggingClient(time.Duration(cfg.HTTPTimeout)*time.Second, logger),operations/sftpgo-authentication-service/internal/service/database.go (1)
42-105: Align DB optionality with constructor behavior.
NewDBServicefails whenDB_CONN_STRINGis empty, butSaveSession/GetSession/DeleteSessiontreat a nil DB as a non-fatal optional mode. Consider either making DB mandatory (remove nil-paths) or allowing a nil DBService when the connection string is unset to keep behavior consistent.operations/sftpgo-authentication-service/internal/handler/utils.go (1)
158-163: Static analysis false positive - this is an authenticator ID, not an API key.The string
"QmFzaWNBdXRoZW50aWNhdG9yOkxPQ0FM"is base64-encoded"BasicAuthenticator:LOCAL", which is Asgardeo's identifier for the local basic authenticator. Consider adding a comment to clarify this for future maintainers.📝 Suggested clarification
// Support BasicAuthenticator where username is mandatory + // "QmFzaWNBdXRoZW50aWNhdG9yOkxPQ0FM" is base64("BasicAuthenticator:LOCAL") if selectedAuth.AuthenticatorID == "QmFzaWNBdXRoZW50aWNhdG9yOkxPQ0FM" {
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@operations/sftpgo-authentication-service/internal/handler/handler.go`:
- Around line 106-110: The debug log in handler.go exposes the raw project key
when extracting parts from u.Username (variables providedProjectKey and
username) via h.logger.Debug; change this to redact or mask the project key
before logging (e.g., replace with "REDACTED" or show only a short prefix/suffix
or hashed value) and log the masked value instead of providedProjectKey while
still logging username, ensuring h.logger.Debug is called with the
maskedProjectKey variable rather than the raw providedProjectKey.
- Around line 190-195: The code currently proceeds to build vfs and perms from
the folders slice even when ProvisionFolders (and its validateFolderName)
returned an error; change the flow in handler.go so that after calling
ProvisionFolders you check its error and immediately return that error (or
otherwise abort) before iterating over folders to construct vfs, perms,
virtualPath and mappedPath (references: ProvisionFolders, validateFolderName,
folders, vfs, perms, generalFileMgtPermissions, h.cfg.FolderPath);
alternatively, run validateFolderName on each folder before using it in
filepath.Join and skip any invalid entries to ensure no unvalidated folder names
are used in path operations.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@operations/sftpgo-authentication-service/internal/httpclient/client.go`:
- Around line 58-82: In logRequest and logResponse where io.ReadFull is used to
read into prefix, do not replace the body with an empty reader on non-EOF read
errors when n>0; instead reconstruct the body to include the bytes successfully
read (prefix[:n]) followed by the remaining unread stream using io.MultiReader
(preserve req.Body/resp.Body by wrapping bytes.NewReader(prefix[:n]) plus the
original reader), and update the Trace log to print prefix[:n] (indicating
partial read and the error) so partial data is not lost on read errors.
In `@operations/sftpgo-authentication-service/internal/service/sftpgo.go`:
- Around line 19-27: The code builds SFTPGo API URLs by raw concatenation of
username and folder path segments which can break on reserved characters; add
the net/url package to the imports and replace string concatenation for path
segments with url.PathEscape(...) when composing any API paths that include user
names or folder paths (e.g., the places where you currently append username or
home/folder variables into the request URL), ensuring all path segments are
escaped before joining into the final URL.
In `@operations/sftpgo-authentication-service/internal/util/util.go`:
- Around line 34-60: InitEmailRegex currently assigns emailRegex even when
regexp.Compile fails, leaving it nil and causing IsLikelyEmail to panic; change
InitEmailRegex to only set the package-level emailRegex after regexp.Compile
returns nil error (preserve existing default fallback behavior), and update
IsLikelyEmail to defensively check that emailRegex != nil before calling
MatchString (return false if nil). Reference: InitEmailRegex, emailRegex, and
IsLikelyEmail.
🧹 Nitpick comments (1)
operations/sftpgo-authentication-service/internal/util/util.go (1)
70-74: Use a clearer error for empty folder names.
io.ErrShortBufferis misleading for validation; a descriptive error will be easier to handle and log.♻️ Suggested tweak
if name == "" { - return io.ErrShortBuffer // Reusing generic error for empty + return fmt.Errorf("invalid folder name: empty") }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@operations/sftpgo-authentication-service/internal/handler/utils.go`:
- Line 187: The call to h.db.SaveSession(session.RequestID,
models.SessionData{FlowID: session.FlowID, NextStep: idpResp.NextStep}) is not
checking for errors; add error handling similar to handleAuthStep1: capture the
returned error from h.db.SaveSession, log it (using the existing logger in the
handler), and return or propagate an appropriate handler error/response so the
flow stops on persistence failures. Locate the SaveSession invocation in
utils.go and wrap it with an if err := h.db.SaveSession(...); err != nil { /*
log and return error response */ } pattern consistent with other handler
functions.
- Around line 115-118: The call to h.db.SaveSession(...) ignores errors—update
the code around h.db.SaveSession(req.RequestID, models.SessionData{FlowID:
idpResp.FlowID, NextStep: idpResp.NextStep}) to capture the returned error, log
it with context (include req.RequestID and the session data), and return/fail
early from the enclosing handler function instead of continuing; ensure you
surface a suitable error response to the caller so downstream steps that expect
the session do not proceed when persistence fails.
🧹 Nitpick comments (5)
operations/sftpgo-authentication-service/internal/config/config.go (2)
86-92:IsSensitivefield is defined but never used.The
EnvVarstruct has anIsSensitivefield that is populated invalidateEnvVarsbut never read. This appears to be scaffolding for future functionality (e.g., masking sensitive values in logs). Consider either implementing the intended functionality or removing the unused field to avoid confusion.
127-163: Silent fallback on invalid timeout values is acceptable but could mask misconfigurations.The timeout parsing blocks (e.g.,
HTTP_TIMEOUT,READ_TIMEOUT) silently fall back to defaults when parsing fails. While this is a reasonable approach for resilience, consider logging a warning when an invalid value is provided so operators can detect misconfigurations.💡 Optional: Add warning for invalid values
if timeoutStr := os.Getenv("HTTP_TIMEOUT"); timeoutStr != "" { - if timeout, err := strconv.Atoi(timeoutStr); err == nil && timeout > 0 { + timeout, err := strconv.Atoi(timeoutStr) + if err != nil || timeout <= 0 { + // Consider logging: fmt.Fprintf(os.Stderr, "Warning: invalid HTTP_TIMEOUT '%s', using default\n", timeoutStr) + } else { cfg.HTTPTimeout = timeout } }operations/sftpgo-authentication-service/internal/service/idp.go (2)
97-106: SCIM filter escaping may be incomplete for special characters.The current escaping only handles double quotes (
"). SCIM filter syntax may also be affected by other special characters like backslashes. Consider a more comprehensive escaping approach.💡 Suggested improvement
asgUser := username if !strings.Contains(username, "/") { asgUser = "DEFAULT/" + username } -// Security: Escape quotes in username to prevent SCIM filter injection -safeUsername := strings.ReplaceAll(asgUser, `"`, `\"`) +// Security: Escape special characters to prevent SCIM filter injection +safeUsername := strings.ReplaceAll(asgUser, `\`, `\\`) +safeUsername = strings.ReplaceAll(safeUsername, `"`, `\"`) filter := fmt.Sprintf(`userName eq "%s"`, safeUsername)
185-214: Consider returning error on HTTP 4xx/5xx in PostToAuthnEndpoint.Currently, when
res.StatusCode >= 400, the method logs a warning but still attempts to unmarshal and return the response. While this may be intentional to capture IdP error details inidpResp.Error, callers might not consistently check for error conditions in the returned struct.operations/sftpgo-authentication-service/internal/handler/handler.go (1)
279-294: API key comparison may be vulnerable to timing attacks.The direct string comparison
apiKey != h.cfg.HookAPIKeycould potentially leak timing information. For security-critical API key validation, consider using constant-time comparison.💡 Suggested improvement
+import "crypto/subtle" + func (h *Handler) authenticate(r *http.Request, w http.ResponseWriter) bool { if h.cfg.HookAPIKey == "" { return true } apiKey := r.Header.Get("API-Key") - if apiKey != h.cfg.HookAPIKey { + if subtle.ConstantTimeCompare([]byte(apiKey), []byte(h.cfg.HookAPIKey)) != 1 { h.logger.Warn("Unauthorized access attempt from %s: invalid or missing API key", r.RemoteAddr)
rksk
commented
Mar 25, 2026
v15a1
reviewed
Mar 25, 2026
v15a1
reviewed
Mar 25, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
operations/sftpgo-authentication-service/internal/service/database.go (1)
86-89: Consider making session TTL configurable.The 15-minute session expiry is hardcoded. For keyboard-interactive MFA flows, this should be sufficient, but making it configurable via environment variable would provide operational flexibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/sftpgo-authentication-service/internal/service/database.go` around lines 86 - 89, Replace the hardcoded 15-minute TTL when computing expiresAt with a configurable value read from an environment/config setting (e.g., SESSION_TTL_MINUTES or SESSION_TTL); parse it (with strconv.Atoi or strconv.ParseInt), fall back to 15 if unset/invalid, then compute expiresAt := time.Now().Add(time.Duration(ttlMinutes) * time.Minute) before executing the existing INSERT/ON DUPLICATE KEY UPDATE query (referencing the expiresAt variable and the query string in this file).operations/sftpgo-authentication-service/.env.example (1)
30-31: Add space between comment header and description.The comment on line 30 has the header merged with the description text. This should be formatted consistently with other sections.
🔧 Proposed fix
-#Database Connection (MySQL)Format: user:password@tcp(host:port)/dbname +# Database Connection (MySQL) +# Format: user:password@tcp(host:port)/dbname🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/sftpgo-authentication-service/.env.example` around lines 30 - 31, The comment header is merged with its description; update the comment above DB_CONN_STRING so it matches other sections by adding a space after the leading "#" and separating the header from the description (e.g., split "Database Connection (MySQL)" and "Format: ..." with a space or punctuation), keeping the same wording and ensuring the DB_CONN_STRING example remains unchanged.operations/sftpgo-authentication-service/internal/service/idp.go (1)
108-114: SCIM filter escaping may be insufficient for all edge cases.The current escaping only handles double quotes. SCIM filter syntax may have other special characters (backslashes, parentheses) that could cause issues. Consider using a more robust escaping approach.
🔧 Proposed fix
asgUser := username if !strings.Contains(username, "/") { asgUser = "DEFAULT/" + username } - // Security: Escape quotes in username to prevent SCIM filter injection - safeUsername := strings.ReplaceAll(asgUser, `"`, `\"`) + // Security: Escape special characters in username to prevent SCIM filter injection + safeUsername := strings.ReplaceAll(asgUser, `\`, `\\`) + safeUsername = strings.ReplaceAll(safeUsername, `"`, `\"`) filter := fmt.Sprintf(`userName eq "%s"`, safeUsername)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/sftpgo-authentication-service/internal/service/idp.go` around lines 108 - 114, The SCIM filter escaping in idp.go is currently only replacing double quotes and can miss other special characters (e.g., backslashes and parentheses); update the safeUsername construction used for filter (asgUser -> safeUsername) to perform robust escaping by replacing backslash, double-quote, open- and close-parentheses (and any other SCIM-special characters your policy requires) with their backslash-escaped equivalents, e.g., use a single strings.NewReplacer that maps `\`->`\\`, `"`->`\"`, `(`->`\(`, `)`->`\)` before building filter := fmt.Sprintf(`userName eq "%s"`, safeUsername) so the resulting SCIM filter cannot be broken by those characters.operations/sftpgo-authentication-service/internal/service/sftpgo.go (1)
189-196: Use forward slashes explicitly formapped_pathwhen constructing server-side paths.
filepath.Joinuses OS-specific separators. While typical deployments run both the service and SFTPGo on Linux (where this produces correct POSIX paths), if SFTPGo is deployed on Windows while this service runs on Linux, the path will contain forward slashes that Windows may not interpret correctly. To ensure cross-platform compatibility, construct the path using forward slashes explicitly instead offilepath.Join:path := s.cfg.FolderPath + "/" + nameThis is especially important since the path is sent to SFTPGo's API for interpretation on a potentially different system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/sftpgo-authentication-service/internal/service/sftpgo.go` around lines 189 - 196, The code in createFolder uses filepath.Join to build the mapped_path which uses OS-specific separators; replace that with explicit POSIX-style joining so mapped_path always uses forward slashes (e.g., combine s.cfg.FolderPath and name with "/" between them) before placing it in the payload for the mapped_path field; update the createFolder function to construct path using a forward-slash join instead of filepath.Join to ensure the path sent to SFTPGo is interpreted correctly on a different OS.operations/sftpgo-authentication-service/README.md (2)
23-37: Add language identifier to fenced code block.The architecture diagram code block lacks a language specifier. For ASCII diagrams, use
textorplaintextto satisfy linting.🔧 Suggested fix
-``` +```text ┌─────────────┐ ┌──────────────────┐ ┌─────────────────┐🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/sftpgo-authentication-service/README.md` around lines 23 - 37, The fenced ASCII diagram block is missing a language identifier; update the opening fence for the diagram (the triple backticks that start the ASCII art block) to include a plaintext language tag (e.g., change ``` to ```text) so the diagram is recognized as plain text by linters and renderers.
41-62: Add language identifier to fenced code block.The project structure code block lacks a language specifier. Use
textorplaintextfor directory listings.🔧 Suggested fix
-``` +```text . ├── cmd/server/main.go # Application entry point🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/sftpgo-authentication-service/README.md` around lines 41 - 62, The fenced directory listing in README.md is missing a language identifier for the code block; update the opening triple-backticks to include a language tag like "text" or "plaintext" (i.e., change ``` to ```text) so the directory tree renders correctly, leaving the content of the listing unchanged and only modifying the code fence in README.md.operations/sftpgo-authentication-service/internal/handler/utils.go (3)
209-213: Consider logging DeleteSession failures for observability.While session cleanup failures shouldn't block authentication success, silently ignoring them may hide persistent storage issues.
🔧 Suggested improvement
func (h *Handler) handleAuthSuccess(resp *models.KeyIntResponse, req *models.KeyIntRequest) { resp.AuthResult = AuthResultSuccess - h.db.DeleteSession(req.RequestID) + if err := h.db.DeleteSession(req.RequestID); err != nil { + h.logger.Warn("Failed to delete session %s after successful auth: %v", req.RequestID, err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/sftpgo-authentication-service/internal/handler/utils.go` around lines 209 - 213, In handleAuthSuccess, after setting resp.AuthResult = AuthResultSuccess and calling h.db.DeleteSession(req.RequestID), capture the returned error from h.db.DeleteSession and log it (using the handler's logger or process logger) without changing the success flow; i.e., do not return or alter resp on failure—just record the error for observability. Ensure you reference the Handler.handleAuthSuccess method and the h.db.DeleteSession call so the log includes the RequestID and the error message for easier debugging.
109-115: Same issue:errmay be nil when logged.If the condition fails because
idpResp.FlowStatus == "FAILED"oridpResp.NextStep == nil,errwould be nil but still logged.🔧 Suggested fix
idpResp, err := h.idp.PostToAuthnEndpoint(discoveryPayload, util.IsInternalUser(req.Username, h.cfg.InternalUserSuffix)) if err != nil || idpResp.FlowStatus == "FAILED" || idpResp.NextStep == nil { resp.AuthResult = AuthResultFailure resp.Instruction = "Authentication failed." - h.logger.Error("IdP discovery failed: %v", err) + if err != nil { + h.logger.Error("IdP discovery failed: %v", err) + } else { + h.logger.Error("IdP discovery failed: FlowStatus=%s, NextStep=%v", idpResp.FlowStatus, idpResp.NextStep) + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/sftpgo-authentication-service/internal/handler/utils.go` around lines 109 - 115, The error logging after calling h.idp.PostToAuthnEndpoint incorrectly assumes err is always set; update the failure handling in the block that checks the result of h.idp.PostToAuthnEndpoint (variables idpResp and err) so you only log err when err != nil, and otherwise include relevant idpResp details (e.g., idpResp.FlowStatus and whether idpResp.NextStep is nil) in the log message; ensure resp.AuthResult and resp.Instruction are still set and the function returns in all failure cases.
92-98: Misleading error log whenerris nil.The condition can fail due to
initFlowResp.NextStep == nilor empty authenticators whileerrisnil. Loggingerrin that case outputs "nil" which is misleading for debugging.🔧 Suggested fix
initFlowResp, err := h.idp.InitFlow(req.Username) if err != nil || initFlowResp.NextStep == nil || len(initFlowResp.NextStep.Authenticators) == 0 { resp.AuthResult = AuthResultFailure resp.Instruction = "Authentication failed: Error initiating flow." - h.logger.Error("Failed to get initial flow from IdP: %v", err) + if err != nil { + h.logger.Error("Failed to get initial flow from IdP: %v", err) + } else { + h.logger.Error("Failed to get initial flow from IdP: invalid response (nil NextStep or no authenticators)") + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operations/sftpgo-authentication-service/internal/handler/utils.go` around lines 92 - 98, The current check after calling h.idp.InitFlow(req.Username) logs err even when err==nil (e.g., when initFlowResp.NextStep==nil or authenticators are empty), which is misleading; update the conditional handling around InitFlow so that when err != nil you log the error from InitFlow (using h.logger.Error with the err), and when err == nil but initFlowResp == nil or initFlowResp.NextStep == nil or len(initFlowResp.NextStep.Authenticators) == 0 you log a clear, distinct message (e.g., "InitFlow returned empty/invalid response" or mention which field is missing) while still setting resp.AuthResult = AuthResultFailure and resp.Instruction accordingly; locate and modify the block referencing h.idp.InitFlow, initFlowResp, NextStep, Authenticators, and h.logger.Error to implement these two separate log paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operations/sftpgo-authentication-service/internal/service/subscription.go`:
- Around line 46-52: The NewSubscriptionService constructor currently creates
the HTTP client with a hardcoded 10-second timeout; change it to use the
configured timeout value (cfg.HTTPTimeout) so it matches
NewIdPService/NewSFTPGoService behavior — update the call to
httpclient.NewLoggingClient in NewSubscriptionService to pass cfg.HTTPTimeout
(and keep using logger) so SubscriptionService.client honors the configured HTTP
timeout.
---
Nitpick comments:
In `@operations/sftpgo-authentication-service/.env.example`:
- Around line 30-31: The comment header is merged with its description; update
the comment above DB_CONN_STRING so it matches other sections by adding a space
after the leading "#" and separating the header from the description (e.g.,
split "Database Connection (MySQL)" and "Format: ..." with a space or
punctuation), keeping the same wording and ensuring the DB_CONN_STRING example
remains unchanged.
In `@operations/sftpgo-authentication-service/internal/handler/utils.go`:
- Around line 209-213: In handleAuthSuccess, after setting resp.AuthResult =
AuthResultSuccess and calling h.db.DeleteSession(req.RequestID), capture the
returned error from h.db.DeleteSession and log it (using the handler's logger or
process logger) without changing the success flow; i.e., do not return or alter
resp on failure—just record the error for observability. Ensure you reference
the Handler.handleAuthSuccess method and the h.db.DeleteSession call so the log
includes the RequestID and the error message for easier debugging.
- Around line 109-115: The error logging after calling h.idp.PostToAuthnEndpoint
incorrectly assumes err is always set; update the failure handling in the block
that checks the result of h.idp.PostToAuthnEndpoint (variables idpResp and err)
so you only log err when err != nil, and otherwise include relevant idpResp
details (e.g., idpResp.FlowStatus and whether idpResp.NextStep is nil) in the
log message; ensure resp.AuthResult and resp.Instruction are still set and the
function returns in all failure cases.
- Around line 92-98: The current check after calling
h.idp.InitFlow(req.Username) logs err even when err==nil (e.g., when
initFlowResp.NextStep==nil or authenticators are empty), which is misleading;
update the conditional handling around InitFlow so that when err != nil you log
the error from InitFlow (using h.logger.Error with the err), and when err == nil
but initFlowResp == nil or initFlowResp.NextStep == nil or
len(initFlowResp.NextStep.Authenticators) == 0 you log a clear, distinct message
(e.g., "InitFlow returned empty/invalid response" or mention which field is
missing) while still setting resp.AuthResult = AuthResultFailure and
resp.Instruction accordingly; locate and modify the block referencing
h.idp.InitFlow, initFlowResp, NextStep, Authenticators, and h.logger.Error to
implement these two separate log paths.
In `@operations/sftpgo-authentication-service/internal/service/database.go`:
- Around line 86-89: Replace the hardcoded 15-minute TTL when computing
expiresAt with a configurable value read from an environment/config setting
(e.g., SESSION_TTL_MINUTES or SESSION_TTL); parse it (with strconv.Atoi or
strconv.ParseInt), fall back to 15 if unset/invalid, then compute expiresAt :=
time.Now().Add(time.Duration(ttlMinutes) * time.Minute) before executing the
existing INSERT/ON DUPLICATE KEY UPDATE query (referencing the expiresAt
variable and the query string in this file).
In `@operations/sftpgo-authentication-service/internal/service/idp.go`:
- Around line 108-114: The SCIM filter escaping in idp.go is currently only
replacing double quotes and can miss other special characters (e.g., backslashes
and parentheses); update the safeUsername construction used for filter (asgUser
-> safeUsername) to perform robust escaping by replacing backslash,
double-quote, open- and close-parentheses (and any other SCIM-special characters
your policy requires) with their backslash-escaped equivalents, e.g., use a
single strings.NewReplacer that maps `\`->`\\`, `"`->`\"`, `(`->`\(`, `)`->`\)`
before building filter := fmt.Sprintf(`userName eq "%s"`, safeUsername) so the
resulting SCIM filter cannot be broken by those characters.
In `@operations/sftpgo-authentication-service/internal/service/sftpgo.go`:
- Around line 189-196: The code in createFolder uses filepath.Join to build the
mapped_path which uses OS-specific separators; replace that with explicit
POSIX-style joining so mapped_path always uses forward slashes (e.g., combine
s.cfg.FolderPath and name with "/" between them) before placing it in the
payload for the mapped_path field; update the createFolder function to construct
path using a forward-slash join instead of filepath.Join to ensure the path sent
to SFTPGo is interpreted correctly on a different OS.
In `@operations/sftpgo-authentication-service/README.md`:
- Around line 23-37: The fenced ASCII diagram block is missing a language
identifier; update the opening fence for the diagram (the triple backticks that
start the ASCII art block) to include a plaintext language tag (e.g., change ```
to ```text) so the diagram is recognized as plain text by linters and renderers.
- Around line 41-62: The fenced directory listing in README.md is missing a
language identifier for the code block; update the opening triple-backticks to
include a language tag like "text" or "plaintext" (i.e., change ``` to ```text)
so the directory tree renders correctly, leaving the content of the listing
unchanged and only modifying the code fence in README.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 868ed87a-c655-427f-9fdf-ac2fcfd24628
📒 Files selected for processing (15)
operations/sftpgo-authentication-service/.choreo/component.yamloperations/sftpgo-authentication-service/.env.exampleoperations/sftpgo-authentication-service/README.mdoperations/sftpgo-authentication-service/cmd/server/main.gooperations/sftpgo-authentication-service/internal/config/config.gooperations/sftpgo-authentication-service/internal/config/config_test.gooperations/sftpgo-authentication-service/internal/constants/constants.gooperations/sftpgo-authentication-service/internal/handler/handler.gooperations/sftpgo-authentication-service/internal/handler/handler_test.gooperations/sftpgo-authentication-service/internal/handler/utils.gooperations/sftpgo-authentication-service/internal/httpclient/client.gooperations/sftpgo-authentication-service/internal/service/database.gooperations/sftpgo-authentication-service/internal/service/idp.gooperations/sftpgo-authentication-service/internal/service/sftpgo.gooperations/sftpgo-authentication-service/internal/service/subscription.go
✅ Files skipped from review due to trivial changes (1)
- operations/sftpgo-authentication-service/internal/constants/constants.go
🚧 Files skipped from review as they are similar to previous changes (4)
- operations/sftpgo-authentication-service/.choreo/component.yaml
- operations/sftpgo-authentication-service/cmd/server/main.go
- operations/sftpgo-authentication-service/internal/config/config_test.go
- operations/sftpgo-authentication-service/internal/handler/handler.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This PR introduces the SFTPGo Authentication Service, a Go-based middleware designed to handle SFTPGo's
pre-loginandkeyboard-interactiveauthentication hooks. It acts as a secure bridge between SFTPGo and Identity Providers (Asgardeo) to handle user identification, route requests to the correct organization, and handle dynamic directory provisioning.Goals
Approach
User stories
Release note
Initial release of the SFTPGo Authentication Service supporting Asgardeo integration, dynamic role-based provisioning, and multi-step keyboard-interactive authentication.
Documentation
Includes a comprehensive
README.mddetailing the architecture, setup, and environment configurations, along with a fullopenapi.yamlspecification.Training
N/A
Certification
N/A - This change does not impact existing certification exams.
Marketing
N/A
Automation tests
internal/config,internal/handler,internal/service, andinternal/util.Security checks
Samples
The
openapi.yamlfile provides high-level details and request/response samples for the hook endpoints.Related PRs
N/A
Migrations (if applicable)
Includes
db/migrations/001_create_sessions_table.up.sqlto initialize the required MySQL schema for session persistence.Test environment
Learning
Researched SFTPGo external hook protocols and Asgardeo's app-native authentication APIs to implement the multi-step keyboard-interactive flow.
Summary by CodeRabbit