add support for env credentials or loading creds from json file #127
Conversation
|
recheck |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
WalkthroughThe changes add three credential supply methods for LDAP authentication: JSON file path, environment variables, and legacy username/password. A new Changes
Sequence DiagramsequenceDiagram
actor User
participant App as Sharphound
participant JSON as JSON File
participant Env as Environment
participant LDAP as LDAP Connection
User->>App: Start with credential options
alt LDAPCredentialJsonPath provided
App->>JSON: Read JSON file
JSON-->>App: Credentials object
App->>LDAP: Set username/password
else LDAPEnvUsername provided
App->>Env: Check LDAPEnvUsername
Env-->>App: Username value
alt LDAPEnvPassword exists
App->>Env: Check LDAPEnvPassword
Env-->>App: Password value
App->>LDAP: Set username/password
else LDAPEnvPassword missing
App-->>App: ❌ Error & abort
end
else LDAPUsername & LDAPPassword provided
App->>LDAP: Set username/password (legacy)
end
App->>LDAP: Proceed with connection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Sharphound.cs (1)
103-131: Document or enforce mutual exclusivity of credential sources.Multiple credential sources can be specified simultaneously (JSON file, environment variables, and legacy username/password), creating undocumented precedence behavior. Currently, if all three are specified, each overwrites the previous one in sequence, with legacy credentials taking final precedence. This could lead to confusion and unexpected behavior.
Consider adding validation to ensure only one credential source is used:
if (options.DomainController != null) ldapOptions.Server = options.DomainController; + // Validate that only one credential source is specified + int credentialSourceCount = 0; + if (options.LDAPCredentialJsonPath != null) credentialSourceCount++; + if (options.LDAPEnvUsername != null) credentialSourceCount++; + if (options.LDAPUsername != null) credentialSourceCount++; + + if (credentialSourceCount > 1) { + logger.LogError("Multiple credential sources specified. Please use only one of: --ldapcredentialsjsonpath, --ldapenvusername, or --ldapusername"); + return; + } + if (options.LDAPCredentialJsonPath != null) {
🧹 Nitpick comments (1)
src/Sharphound.cs (1)
116-120: Follow C# naming conventions for local variables.Local variables
EnvUsernameandEnvPasswordshould use camelCase rather than PascalCase per C# conventions.Apply this diff:
- string EnvUsername = Environment.GetEnvironmentVariable(options.LDAPEnvUsername); - string EnvPassword = Environment.GetEnvironmentVariable(options.LDAPEnvPassword); + string envUsername = Environment.GetEnvironmentVariable(options.LDAPEnvUsername); + string envPassword = Environment.GetEnvironmentVariable(options.LDAPEnvPassword); - ldapOptions.Username = EnvUsername; - ldapOptions.Password = EnvPassword; + ldapOptions.Username = envUsername; + ldapOptions.Password = envPassword;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)src/JsonExtensions.cs(1 hunks)src/Options.cs(1 hunks)src/Sharphound.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Sharphound.cs (1)
src/JsonExtensions.cs (1)
Credentials(12-16)
🔇 Additional comments (2)
src/JsonExtensions.cs (1)
12-16: LGTM with validation recommendation.The
Credentialsclass design is appropriate for JSON deserialization. However, ensure that validation of the deserialized values (non-null, non-empty) occurs at the usage site.README.md (1)
80-84: Fix typo: "enviroment" → "environment".The word "environment" is misspelled in the documentation for the new LDAP credential options.
Apply this diff:
- --ldapenvusername Username for LDAP stored in a defined environment variable (argument takes the environment variable as input) + --ldapenvusername Username for LDAP stored in a defined environment variable (argument takes the environment variable name as input) - --ldapenvpassword Password for LDAP stored in a defined environment variable (argument takes the environment variable as input) + --ldapenvpassword Password for LDAP stored in a defined environment variable (argument takes the environment variable name as input)Likely an incorrect or invalid review comment.
| [Option(HelpText = "Enviroment variable name for LDAP Username", Default = null)] | ||
| public string LDAPEnvUsername { get; set; } | ||
|
|
||
| [Option(HelpText = "Enviroment variable name for LDAP Password", Default = null)] | ||
| public string LDAPEnvPassword { get; set; } |
There was a problem hiding this comment.
Fix typo: "Enviroment" → "Environment".
The help text contains a spelling error that should be corrected for consistency and professionalism.
Apply this diff:
- [Option(HelpText = "Enviroment variable name for LDAP Username", Default = null)]
+ [Option(HelpText = "Environment variable name for LDAP Username", Default = null)]
public string LDAPEnvUsername { get; set; }
- [Option(HelpText = "Enviroment variable name for LDAP Password", Default = null)]
+ [Option(HelpText = "Environment variable name for LDAP Password", Default = null)]
public string LDAPEnvPassword { get; set; }📝 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.
| [Option(HelpText = "Enviroment variable name for LDAP Username", Default = null)] | |
| public string LDAPEnvUsername { get; set; } | |
| [Option(HelpText = "Enviroment variable name for LDAP Password", Default = null)] | |
| public string LDAPEnvPassword { get; set; } | |
| [Option(HelpText = "Environment variable name for LDAP Username", Default = null)] | |
| public string LDAPEnvUsername { get; set; } | |
| [Option(HelpText = "Environment variable name for LDAP Password", Default = null)] | |
| public string LDAPEnvPassword { get; set; } |
🤖 Prompt for AI Agents
In src/Options.cs around lines 84 to 88, the HelpText strings for
LDAPEnvUsername and LDAPEnvPassword contain a typo "Enviroment"; update both
HelpText values to read "Environment variable name for LDAP Username" and
"Environment variable name for LDAP Password" respectively to correct the
spelling.
| if (options.LDAPCredentialJsonPath != null) { | ||
| string json = File.ReadAllText(options.LDAPCredentialJsonPath); | ||
| Credentials ldapCreds = JsonConvert.DeserializeObject<Credentials>(json); | ||
| ldapOptions.Username = ldapCreds.Username; | ||
| ldapOptions.Password = ldapCreds.Password; | ||
| } |
There was a problem hiding this comment.
Add missing using statements and error handling for JSON credential loading.
The code references File.ReadAllText and JsonConvert.DeserializeObject but the required using statements (System.IO and Newtonsoft.Json) are not present in the file. Additionally, there is no error handling for file operations or JSON deserialization failures.
Apply this diff to add the required using statements at the top of the file:
using System;
using System.DirectoryServices.Protocols;
+using System.IO;
using System.Threading;
using System.Threading.Tasks;
using CommandLine;
using Microsoft.Extensions.Logging;
using Microsoft.Win32;
+using Newtonsoft.Json;
using Sharphound.Client;
using SharpHoundCommonLib;
using SharpHoundCommonLib.Enums;Then apply this diff to add error handling and validation:
if (options.LDAPCredentialJsonPath != null) {
- string json = File.ReadAllText(options.LDAPCredentialJsonPath);
- Credentials ldapCreds = JsonConvert.DeserializeObject<Credentials>(json);
- ldapOptions.Username = ldapCreds.Username;
- ldapOptions.Password = ldapCreds.Password;
+ try {
+ string json = File.ReadAllText(options.LDAPCredentialJsonPath);
+ Credentials ldapCreds = JsonConvert.DeserializeObject<Credentials>(json);
+
+ if (ldapCreds == null || string.IsNullOrEmpty(ldapCreds.Username) || string.IsNullOrEmpty(ldapCreds.Password)) {
+ logger.LogError("Invalid credentials in JSON file. Both Username and Password must be provided.");
+ return;
+ }
+
+ ldapOptions.Username = ldapCreds.Username;
+ ldapOptions.Password = ldapCreds.Password;
+ } catch (FileNotFoundException) {
+ logger.LogError("LDAP credentials JSON file not found at path: {Path}", options.LDAPCredentialJsonPath);
+ return;
+ } catch (JsonException ex) {
+ logger.LogError("Failed to parse LDAP credentials JSON file: {Message}", ex.Message);
+ return;
+ } catch (Exception ex) {
+ logger.LogError("Error reading LDAP credentials from JSON file: {Message}", ex.Message);
+ return;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Sharphound.cs around lines 103 to 108, the code uses File.ReadAllText and
JsonConvert.DeserializeObject but lacks the required using directives and any
error handling; add using System.IO; and using Newtonsoft.Json; at the top of
the file, then wrap the file-read and deserialization block in a try-catch that
catches IO and JSON exceptions (e.g., IOException, JsonException, and a general
Exception fallback), validate that the deserialized Credentials object and its
Username/Password are not null or empty, and on error either log a clear message
and rethrow or throw a new descriptive exception so callers can handle the
failure instead of letting unhandled exceptions propagate silently.
| if (options.LDAPEnvUsername != null) { | ||
| if (options.LDAPEnvPassword == null) { | ||
| logger.LogError("You must specify LDAPEnvPassword if using the LDAPEnvUsername options"); | ||
| return; | ||
| } | ||
|
|
||
| string EnvUsername = Environment.GetEnvironmentVariable(options.LDAPEnvUsername); | ||
| string EnvPassword = Environment.GetEnvironmentVariable(options.LDAPEnvPassword); | ||
|
|
||
| ldapOptions.Username = EnvUsername; | ||
| ldapOptions.Password = EnvPassword; | ||
| } |
There was a problem hiding this comment.
Add validation for environment variable credential values.
The code retrieves credentials from environment variables but doesn't validate that the variables exist or contain non-empty values. If an environment variable doesn't exist, Environment.GetEnvironmentVariable returns null, which would set invalid credentials silently.
Apply this diff:
if (options.LDAPEnvUsername != null) {
if (options.LDAPEnvPassword == null) {
logger.LogError("You must specify LDAPEnvPassword if using the LDAPEnvUsername options");
return;
}
string EnvUsername = Environment.GetEnvironmentVariable(options.LDAPEnvUsername);
string EnvPassword = Environment.GetEnvironmentVariable(options.LDAPEnvPassword);
+
+ if (string.IsNullOrEmpty(EnvUsername)) {
+ logger.LogError("Environment variable {VarName} not found or empty", options.LDAPEnvUsername);
+ return;
+ }
+
+ if (string.IsNullOrEmpty(EnvPassword)) {
+ logger.LogError("Environment variable {VarName} not found or empty", options.LDAPEnvPassword);
+ return;
+ }
ldapOptions.Username = EnvUsername;
ldapOptions.Password = EnvPassword;
}🤖 Prompt for AI Agents
In src/Sharphound.cs around lines 110 to 121, the code reads LDAP credentials
from environment variables but does not validate that the retrieved values are
present or non-empty; update the block to check
Environment.GetEnvironmentVariable(...) results with string.IsNullOrWhiteSpace
for both username and password, and if either is null/empty log a clear error
referencing the offending options (options.LDAPEnvUsername or
options.LDAPEnvPassword) and return early instead of assigning invalid
credentials to ldapOptions.Username/Password.
2 participants
Description
Add argument option to load ldapusername/ldappassword from environment variable (operator define the env variable name to pull from) or a operator defineable json file. I've specifically left out any configuration options or similar as it probably goes to much in conflict with the enterprise version of Sharphound
Motivation and Context
I am not a big fan of spraying passwords in my windows logs. This should hopefully to some extent mitigate that. Do note that this is not to be looked at as a security feature, but it atleast prevents the host from spraying passwords into logs and collected logs.
How Has This Been Tested?
Compiled code, tested with json file and envoptions (also tested the --ldapusername --ldappassword options).
This was tested on 4 different active directory domains. The changes are not huge and didnt impact any of the operations of Sharphound.
Screenshots (if appropriate):
Types of changes
Checklist:
Didnt know there were any tests or where to define them. Im not doing anything towards the database, so that shouldnt be needed to define. Not sure if there are any other places than the readme to update
Im by no means of the imagination a developer, but it would be nice to have this or some similar feature included to ensure that passwords are not sprayed in the logs.
Summary by CodeRabbit
New Features
Documentation