Skip to content

Conversation

NikitaCOEUR
Copy link

@NikitaCOEUR NikitaCOEUR commented Sep 3, 2025

Checklist for Pull Requests


Description :

This pull request adds the ability to hot reload OIDC client secrets from file using SIGHUP signal (Unix) or 'paramchange' service control on Windows.

Key changes:

  • Add ReloadOIDC() function to reload OIDC secrets for all HTTP servers
  • Add ReloadSecret() method to OIDC struct for atomic secret reload
  • Integrate OIDC reload into existing signal handlers
  • Thread-safe server tracking with httpServers slice and mutex
  • Comprehensive test coverage for all reload scenarios

Important limitations:
⚠️ Hot reload is ONLY functional for secrets defined in clientSecretFile
⚠️ Direct clientSecret in config cannot be hot reloaded

…ntSecretFile

This commit adds the ability to hot reload OIDC client secrets from file
using SIGHUP signal (Unix) or 'paramchange' service control on Windows.

Key changes:
- Add ReloadOIDC() function to reload OIDC secrets for all HTTP servers
- Add ReloadSecret() method to OIDC struct for atomic secret reload
- Integrate OIDC reload into existing signal handlers
- Thread-safe server tracking with httpServers slice and mutex
- Comprehensive test coverage for all reload scenarios

Important limitations:
⚠️  Hot reload is ONLY functional for secrets defined in clientSecretFile
⚠️  Direct clientSecret in config cannot be hot reloaded
⚠️  All HTTP servers with OIDC configuration are reloaded simultaneously

Technical details:
- Maintains backward compatibility with existing configurations
- Atomic rollback on reload failure to prevent broken state
- Validates oauth2Config type safety during secret updates
- Optimized locking strategy to minimize performance impact

Tests:
- Added comprehensive test suite covering success and failure scenarios
- 100% code coverage maintained for reload functionality
- Edge cases tested: missing files, empty secrets, type mismatches
@NikitaCOEUR NikitaCOEUR requested a review from drakkan as a code owner September 3, 2025 10:51
@CLAassistant
Copy link

CLAassistant commented Sep 3, 2025

CLA assistant check
All committers have signed the CLA.

@tecosaur
Copy link

tecosaur commented Sep 9, 2025

Given that OIDC secrets are generally not expected to change often (or at all), I'd be interested to hear the motivation for this change.

@NikitaCOEUR
Copy link
Author

Given that OIDC secrets are generally not expected to change often (or at all), I'd be interested to hear the motivation for this change.

In the case of Microsoft Entra ID (formerly Azure AD), OIDC client secrets do have enforced expiration policies. Until 2021 it was possible to create “never expiring” secrets (often represented as 99-year values), but Microsoft has since deprecated that option in the portal.

When creating a client secret through the Entra admin portal, the maximum selectable lifetime is 24 months (Microsoft 365 Dev Blog).

While it’s technically still possible to generate longer-lived secrets using PowerShell or Microsoft Graph API by setting a far-future endDateTime, Microsoft’s current guidance is to avoid this and instead implement regular rotation.

The recommended best practice is to rotate secrets every 6–12 months and, where possible, move to Managed Identities or store credentials in Azure Key Vault to automate rotation.

So while in some OIDC providers client secrets can effectively remain static, in Entra ID they are explicitly time-bounded (max 2 years via the portal). That’s the motivation for ensuring our implementation can handle secret renewal gracefully, rather than assuming immutability.

@tecosaur
Copy link

tecosaur commented Sep 9, 2025

I see, thanks for that elaboration. I can't help but wonder if given that frequency (a few times a year), why switching out the secrets and restarting the sftpgo process isn't good enough?

@NikitaCOEUR
Copy link
Author

I see, thanks for that elaboration. I can't help but wonder if given that frequency (a few times a year), why switching out the secrets and restarting the sftpgo process isn't good enough?

You're right — in principle, rotating the secret manually and restarting the sftpgo process a few times a year wouldn’t be a major issue.

The main reason I’m aiming for this change is to enable fully automated secret renewal without service impact. If a restart is required every time, it effectively means:

  • Scheduling a maintenance window,

  • Forcing active connections to drop, and

  • Making it harder to increase the rotation frequency beyond the minimum (e.g. 6–12 months).

By avoiding restarts and disconnections, we can rotate secrets transparently in the background, which aligns better with security best practices and lets us rotate more often without operational overhead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants