Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-10394] Add new item type ssh key #52

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions src/Api/Vault/Controllers/SyncController.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using Bit.Api.Vault.Models.Response;
using Bit.Core;
using Bit.Core.AdminConsole.Entities;
using Bit.Core.AdminConsole.Enums.Provider;
using Bit.Core.AdminConsole.Repositories;
using Bit.Core.Context;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Exceptions;
Expand All @@ -10,6 +12,7 @@
using Bit.Core.Services;
using Bit.Core.Settings;
using Bit.Core.Tools.Repositories;
using Bit.Core.Vault.Models.Data;
using Bit.Core.Vault.Repositories;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;
Expand All @@ -30,6 +33,8 @@ public class SyncController : Controller
private readonly IPolicyRepository _policyRepository;
private readonly ISendRepository _sendRepository;
private readonly GlobalSettings _globalSettings;
private readonly ICurrentContext _currentContext;
private readonly Version _sshKeyCipherMinimumVersion = new(Constants.SSHKeyCipherMinimumVersion);
Copy link

Choose a reason for hiding this comment

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

style: Consider moving this constant to a centralized location for version-related constants


public SyncController(
IUserService userService,
Expand All @@ -41,7 +46,8 @@ public SyncController(
IProviderUserRepository providerUserRepository,
IPolicyRepository policyRepository,
ISendRepository sendRepository,
GlobalSettings globalSettings)
GlobalSettings globalSettings,
ICurrentContext currentContext)
{
_userService = userService;
_folderRepository = folderRepository;
Expand All @@ -53,6 +59,7 @@ public SyncController(
_policyRepository = policyRepository;
_sendRepository = sendRepository;
_globalSettings = globalSettings;
_currentContext = currentContext;
}

[HttpGet("")]
Expand All @@ -74,7 +81,8 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id,
var hasEnabledOrgs = organizationUserDetails.Any(o => o.Enabled);

var folders = await _folderRepository.GetManyByUserIdAsync(user.Id);
var ciphers = await _cipherRepository.GetManyByUserIdAsync(user.Id, withOrganizations: hasEnabledOrgs);
var allCiphers = await _cipherRepository.GetManyByUserIdAsync(user.Id, withOrganizations: hasEnabledOrgs);
var ciphers = FilterSSHKeys(allCiphers);
var sends = await _sendRepository.GetManyByUserIdAsync(user.Id);

IEnumerable<CollectionDetails> collections = null;
Expand All @@ -95,4 +103,16 @@ await _providerUserRepository.GetManyOrganizationDetailsByUserAsync(user.Id,
collectionCiphersGroupDict, excludeDomains, policies, sends);
return response;
}

private ICollection<CipherDetails> FilterSSHKeys(ICollection<CipherDetails> ciphers)
{
if (_currentContext.ClientVersion >= _sshKeyCipherMinimumVersion && _currentContext.DeviceType != DeviceType.Android && _currentContext.DeviceType != DeviceType.iOS && _currentContext.DeviceType != DeviceType.AndroidAmazon)
Copy link

Choose a reason for hiding this comment

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

style: This condition is complex and may be difficult to maintain. Consider extracting it into a separate method for better readability

{
return ciphers;
}
else
{
return ciphers.Where(c => c.Type != Core.Vault.Enums.CipherType.SSHKey).ToList();
}
}
}
26 changes: 26 additions & 0 deletions src/Api/Vault/Models/CipherSSHKeyModel.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
using Bit.Core.Utilities;
using Bit.Core.Vault.Models.Data;

namespace Bit.Api.Vault.Models;

public class CipherSSHKeyModel
{
public CipherSSHKeyModel() { }
Copy link

Choose a reason for hiding this comment

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

style: Empty constructor might allow creation of invalid objects. Consider making it private if not needed publicly.


public CipherSSHKeyModel(CipherSSHKeyData data)
{
PrivateKey = data.PrivateKey;
PublicKey = data.PublicKey;
KeyFingerprint = data.KeyFingerprint;
}
Comment on lines +10 to +15
Copy link

Choose a reason for hiding this comment

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

logic: Constructor doesn't validate input. Consider adding null checks for data and its properties.


[EncryptedString]
[EncryptedStringLength(5000)]
public string PrivateKey { get; set; }
Comment on lines +17 to +19
Copy link

Choose a reason for hiding this comment

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

style: 5000 characters might be excessive for a private key. Verify if this limit is appropriate.

[EncryptedString]
[EncryptedStringLength(5000)]
public string PublicKey { get; set; }
Comment on lines +20 to +22
Copy link

Choose a reason for hiding this comment

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

style: Public keys are typically shorter than private keys. Consider using a smaller limit.

[EncryptedString]
[EncryptedStringLength(1000)]
public string KeyFingerprint { get; set; }
Comment on lines +23 to +25
Copy link

Choose a reason for hiding this comment

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

style: Key fingerprints are usually much shorter. 1000 characters seems excessive.

}
19 changes: 19 additions & 0 deletions src/Api/Vault/Models/Request/CipherRequestModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public class CipherRequestModel
public CipherCardModel Card { get; set; }
public CipherIdentityModel Identity { get; set; }
public CipherSecureNoteModel SecureNote { get; set; }
public CipherSSHKeyModel SSHKey { get; set; }
public DateTime? LastKnownRevisionDate { get; set; } = null;

public CipherDetails ToCipherDetails(Guid userId, bool allowOrgIdSet = true)
Expand Down Expand Up @@ -82,6 +83,9 @@ public Cipher ToCipher(Cipher existingCipher)
case CipherType.SecureNote:
existingCipher.Data = JsonSerializer.Serialize(ToCipherSecureNoteData(), JsonHelpers.IgnoreWritingNull);
break;
case CipherType.SSHKey:
existingCipher.Data = JsonSerializer.Serialize(ToCipherSSHKeyData(), JsonHelpers.IgnoreWritingNull);
break;
default:
throw new ArgumentException("Unsupported type: " + nameof(Type) + ".");
}
Expand Down Expand Up @@ -230,6 +234,21 @@ private CipherSecureNoteData ToCipherSecureNoteData()
Type = SecureNote.Type,
};
}

private CipherSSHKeyData ToCipherSSHKeyData()
{
return new CipherSSHKeyData
{
Name = Name,
Notes = Notes,
Fields = Fields?.Select(f => f.ToCipherFieldData()),
PasswordHistory = PasswordHistory?.Select(ph => ph.ToCipherPasswordHistoryData()),

PrivateKey = SSHKey.PrivateKey,
PublicKey = SSHKey.PublicKey,
KeyFingerprint = SSHKey.KeyFingerprint,
};
}
}

public class CipherWithIdRequestModel : CipherRequestModel
Expand Down
7 changes: 7 additions & 0 deletions src/Api/Vault/Models/Response/CipherResponseModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ public CipherMiniResponseModel(Cipher cipher, IGlobalSettings globalSettings, bo
cipherData = identityData;
Identity = new CipherIdentityModel(identityData);
break;
case CipherType.SSHKey:
var sshKeyData = JsonSerializer.Deserialize<CipherSSHKeyData>(cipher.Data);
Data = sshKeyData;
cipherData = sshKeyData;
SSHKey = new CipherSSHKeyModel(sshKeyData);
break;
Comment on lines +51 to +56
Copy link

Choose a reason for hiding this comment

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

style: Consider adding a null check for sshKeyData after deserialization to prevent potential null reference exceptions

default:
throw new ArgumentException("Unsupported " + nameof(Type) + ".");
}
Expand Down Expand Up @@ -76,6 +82,7 @@ public CipherMiniResponseModel(Cipher cipher, IGlobalSettings globalSettings, bo
public CipherCardModel Card { get; set; }
public CipherIdentityModel Identity { get; set; }
public CipherSecureNoteModel SecureNote { get; set; }
public CipherSSHKeyModel SSHKey { get; set; }
Copy link

Choose a reason for hiding this comment

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

style: Ensure that the SSHKey property is properly documented with XML comments for consistency with other properties

public IEnumerable<CipherFieldModel> Fields { get; set; }
public IEnumerable<CipherPasswordHistoryModel> PasswordHistory { get; set; }
public IEnumerable<AttachmentResponseModel> Attachments { get; set; }
Expand Down
2 changes: 2 additions & 0 deletions src/Core/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public static class Constants
public const int OrganizationSelfHostSubscriptionGracePeriodDays = 60;

public const string Fido2KeyCipherMinimumVersion = "2023.10.0";
public const string SSHKeyCipherMinimumVersion = "2024.7.0";

/// <summary>
/// Used by IdentityServer to identify our own provider.
Expand Down Expand Up @@ -131,6 +132,7 @@ public static class FeatureFlagKeys
public const string AC2828_ProviderPortalMembersPage = "AC-2828_provider-portal-members-page";
public const string ProviderClientVaultPrivacyBanner = "ac-2833-provider-client-vault-privacy-banner";
public const string DeviceTrustLogging = "pm-8285-device-trust-logging";
public const string SSHKeyItemVaultItem = "ssh-key-vault-item";

public static List<string> GetAllKeys()
{
Expand Down
1 change: 1 addition & 0 deletions src/Core/Vault/Enums/CipherType.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ public enum CipherType : byte
SecureNote = 2,
Card = 3,
Identity = 4,
SSHKey = 5,
}
10 changes: 10 additions & 0 deletions src/Core/Vault/Models/Data/CipherSSHKeyData.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
namespace Bit.Core.Vault.Models.Data;

public class CipherSSHKeyData : CipherData
Copy link

Choose a reason for hiding this comment

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

style: Add XML documentation for the class and its purpose

{
public CipherSSHKeyData() { }

public string PrivateKey { get; set; }
public string PublicKey { get; set; }
public string KeyFingerprint { get; set; }
Comment on lines +7 to +9
Copy link

Choose a reason for hiding this comment

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

style: Consider using [EncryptedString] attribute for sensitive data

}