feat: Adding federated identity credentials#164
feat: Adding federated identity credentials#164hotnops wants to merge 4 commits intoSpecterOps:mainfrom
Conversation
Merging from main
WalkthroughAdds listing support for Azure AD Application Federated Identity Credentials: client method, interface and mock updates, new models and enum, a Cobra CLI command to stream FICs with concurrent workers, and integration into the existing list pipeline. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Client as Azure Client
participant API as Azure Graph API
participant Pipeline as Processing Pipeline
CLI->>Client: create client, ctx
CLI->>Pipeline: start listAppFICs workers
Pipeline->>Client: ListAzureADAppFICs(appId, params)
Client->>API: GET /applications/{appId}/federatedIdentityCredentials
API-->>Client: returns FIC items (paged)
Client-->>Pipeline: stream AzureResult[json.RawMessage] via channel
Pipeline->>Pipeline: accumulate into models.AppFICs, wrap in azureWrapper
Pipeline-->>CLI: emit to output channel
Pipeline-->>CLI: close channel when workers complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@client/apps.go`:
- Around line 63-76: The path for the federatedIdentityCredentials endpoint in
ListAzureADAppFICs currently includes a trailing slash; remove the trailing
slash from the formatted path string in function ListAzureADAppFICs (the
variable path built with fmt.Sprintf) so it matches the other Graph calls and
Microsoft docs (use "/%s/applications/%s/federatedIdentityCredentials" instead
of the version that ends with "/").
In `@cmd/list-app-federated-identity-credentials.go`:
- Around line 84-87: The log message inside the loop over
client.ListAzureADAppFICs(ctx, app.Data.Id, params) is misleadingly referencing
“owners”; update the process so that when item.Error != nil the processLogger
(or log) message reflects Federated Identity Credentials (FIC) collection
instead—e.g., change "unable to continue processing owners for this app" to
something like "unable to continue processing federated identity credentials for
this app" or "unable to process FICs for this app", keeping the existing fields
("appId", app.Data.AppId) and the Error payload; locate the loop over
ListAzureADAppFICs and replace only the message text.
In `@models/app-fic.go`:
- Around line 29-38: The custom MarshalJSON in AppFIC calls OmitEmpty on s.FIC
which errors on nil/empty json.RawMessage; update AppFIC.MarshalJSON to first
check s.FIC (e.g., len(s.FIC) == 0 or s.FIC == nil) and if empty skip calling
OmitEmpty and do not set "fic" in the output map, otherwise call OmitEmpty on
s.FIC and set output["fic"] from its result before json.Marshal(output); keep
the rest of the method (setting "appId" and returning json.Marshal(output))
unchanged.
| func (s *AppFIC) MarshalJSON() ([]byte, error) { | ||
| output := make(map[string]any) | ||
| output["appId"] = s.AppId | ||
|
|
||
| if fic, err := OmitEmpty(s.FIC); err != nil { | ||
| return nil, err | ||
| } else { | ||
| output["fic"] = fic | ||
| return json.Marshal(output) | ||
| } |
There was a problem hiding this comment.
Guard empty FIC to avoid marshal errors.
OmitEmpty will error on empty/nil json.RawMessage. If FIC is optional, skip the call when empty and only include the field when present.
🛠️ Proposed fix
func (s *AppFIC) MarshalJSON() ([]byte, error) {
output := make(map[string]any)
output["appId"] = s.AppId
+ if len(s.FIC) == 0 {
+ return json.Marshal(output)
+ }
+
if fic, err := OmitEmpty(s.FIC); err != nil {
return nil, err
} else {
output["fic"] = fic
return json.Marshal(output)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *AppFIC) MarshalJSON() ([]byte, error) { | |
| output := make(map[string]any) | |
| output["appId"] = s.AppId | |
| if fic, err := OmitEmpty(s.FIC); err != nil { | |
| return nil, err | |
| } else { | |
| output["fic"] = fic | |
| return json.Marshal(output) | |
| } | |
| func (s *AppFIC) MarshalJSON() ([]byte, error) { | |
| output := make(map[string]any) | |
| output["appId"] = s.AppId | |
| if len(s.FIC) == 0 { | |
| return json.Marshal(output) | |
| } | |
| if fic, err := OmitEmpty(s.FIC); err != nil { | |
| return nil, err | |
| } else { | |
| output["fic"] = fic | |
| return json.Marshal(output) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@models/app-fic.go` around lines 29 - 38, The custom MarshalJSON in AppFIC
calls OmitEmpty on s.FIC which errors on nil/empty json.RawMessage; update
AppFIC.MarshalJSON to first check s.FIC (e.g., len(s.FIC) == 0 or s.FIC == nil)
and if empty skip calling OmitEmpty and do not set "fic" in the output map,
otherwise call OmitEmpty on s.FIC and set output["fic"] from its result before
json.Marshal(output); keep the rest of the method (setting "appId" and returning
json.Marshal(output)) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/list-app-federated-identity-credentials.go`:
- Around line 20-25: The code registers signals using os.Kill which is
uncatchable; update the signal handling to use syscall.SIGTERM instead of
os.Kill so graceful shutdown works in containers: replace any use of os.Kill
passed to signal.Notify (e.g., signal.Notify(sigCh, os.Interrupt, os.Kill)) with
syscall.SIGTERM (e.g., signal.Notify(sigCh, os.Interrupt, syscall.SIGTERM)) and
add the syscall import; ensure the same change is applied to the other
occurrence around lines 48-50 referencing signal.Notify or os.Kill.
| import ( | ||
| "context" | ||
| "os" | ||
| "os/signal" | ||
| "sync" | ||
| "time" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go os.Kill SIGKILL cannot be trapped caught signal.NotifyContext
💡 Result:
On Unix-like systems, SIGKILL (and SIGSTOP) cannot be caught, blocked, or ignored, so Go cannot “trap” it—neither signal.Notify nor signal.NotifyContext will ever deliver SIGKILL to your program for graceful shutdown. The Go os/signal docs state this explicitly. [1] (This is an OS/POSIX rule, not a Go limitation. [2])
Also, in Go os.Kill is SIGKILL (and (*os.Process).Kill() “causes the Process to exit immediately”), so anything that sends os.Kill/Process.Kill() is inherently non-graceful. [3]
What to do instead (graceful shutdown)
Use a catchable signal like SIGINT/SIGTERM:
ctx, stop := signal.NotifyContext(context.Background(),
os.Interrupt, syscall.SIGTERM,
)
defer stop()
<-ctx.Done() // start shutdown/cleanupThen, if you need a “hard stop” fallback, do it from outside the process (service manager / supervisor): send SIGTERM, wait a grace period, then send SIGKILL.
Sources: Go os/signal docs on SIGKILL/SIGSTOP [1], POSIX signal rules (SIGKILL can’t be caught/ignored) [2], Go os docs showing Kill = SIGKILL / immediate exit semantics [3].
[1] https://pkg.go.dev/os/signal
[2] https://manual.cs50.io/7/signal
[3] https://pkg.go.dev/github.com/s1s1ty/go/src/os
🌐 Web query:
Go graceful shutdown SIGTERM syscall signal handling
💡 Result:
In Go, you typically implement “graceful shutdown” by catching SIGTERM/SIGINT and then cancelling work + calling (*http.Server).Shutdown(ctx) with a timeout.
Why this works
- By default,
SIGINT/SIGTERMterminate the program, butsignal.Notify/signal.NotifyContextdisable the default behavior and deliver the signal to your code so you can clean up. [1] http.Server.Shutdownstops accepting new connections, closes listeners/idle conns, and waits for in-flight requests until the context expires. (It does not handle hijacked conns like WebSockets automatically.) [2]
Minimal pattern (recommended): signal.NotifyContext
package main
import (
"context"
"log"
"net/http"
"os"
"os/signal"
"syscall"
"time"
)
func main() {
srv := &http.Server{Addr: ":8080"}
// Cancel this context when SIGINT (Ctrl+C) or SIGTERM is received.
ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
defer stop()
go func() {
if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed {
log.Fatalf("listen: %v", err)
}
}()
<-ctx.Done() // wait for signal
// Give outstanding requests time to finish.
shutdownCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
if err := srv.Shutdown(shutdownCtx); err != nil {
log.Printf("shutdown: %v", err)
}
}Notes
- In containers/orchestrators (Docker/systemd/Kubernetes),
SIGTERMis the common “please stop” signal, so include it. [3] - You can’t gracefully handle
SIGKILL(it cannot be caught/ignored); onlySIGTERM(then possibly a laterSIGKILLif you don’t exit in time).
Sources:
[1] Go os/signal docs (signal.Notify behavior, incl. SIGTERM) https://go.dev/src/os/signal/doc.go
[2] Example + http.Server.Shutdown behavior notes (incl. WebSockets/hijacked conns) https://rafallorenz.com/go/handle-signals-to-graceful-shutdown-http-server/
[3] Typical use of SIGTERM for shutdown (e.g., Docker/systemd/K8s) https://m.php.cn/en/faq/1796844688.html
🏁 Script executed:
# Check the actual implementation at the mentioned lines
head -60 cmd/list-app-federated-identity-credentials.go | tail -45Repository: SpecterOps/AzureHound
Length of output: 1403
Replace os.Kill with syscall.SIGTERM for proper graceful shutdown in containerized environments.
os.Kill (SIGKILL) cannot be trapped—the OS prevents it. Without SIGTERM, graceful shutdown fails when orchestrators (Docker, Kubernetes, systemd) send the standard termination signal. Only os.Interrupt (SIGINT) will trigger cleanup, which won't occur during normal container shutdown.
Proposed fix
import (
"context"
"os"
"os/signal"
"sync"
+ "syscall"
"time"- ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, os.Kill)
+ ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM)Also applies to: 48-50
🤖 Prompt for AI Agents
In `@cmd/list-app-federated-identity-credentials.go` around lines 20 - 25, The
code registers signals using os.Kill which is uncatchable; update the signal
handling to use syscall.SIGTERM instead of os.Kill so graceful shutdown works in
containers: replace any use of os.Kill passed to signal.Notify (e.g.,
signal.Notify(sigCh, os.Interrupt, os.Kill)) with syscall.SIGTERM (e.g.,
signal.Notify(sigCh, os.Interrupt, syscall.SIGTERM)) and add the syscall import;
ensure the same change is applied to the other occurrence around lines 48-50
referencing signal.Notify or os.Kill.
1 participant
Adding collection of federated identity credentials to the azurehound collector.
Summary by CodeRabbit
New Features
Tests