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-1339] Allow Rotating Device Keys #3096

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
14 changes: 14 additions & 0 deletions src/Api/Auth/Models/Request/UpdateDevicesTrustRequestModel.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
using System.ComponentModel.DataAnnotations;
using Bit.Api.Auth.Models.Request.Accounts;
using Bit.Core.Auth.Models.Api.Request;

#nullable enable

namespace Bit.Api.Auth.Models.Request;

public class UpdateDevicesTrustRequestModel : SecretVerificationRequestModel
{
[Required]
public DeviceKeysUpdateRequestModel CurrentDevice { get; set; } = null!;
public IEnumerable<OtherDeviceKeysUpdateRequestModel>? OtherDevices { get; set; }
}
61 changes: 59 additions & 2 deletions src/Api/Controllers/DevicesController.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
using Bit.Api.Models.Request;
using Bit.Api.Auth.Models.Request;
using Bit.Api.Auth.Models.Request.Accounts;
using Bit.Api.Models.Request;
using Bit.Api.Models.Response;
using Bit.Core.Auth.Models.Api.Request;
using Bit.Core.Auth.Models.Api.Response;
using Bit.Core.Context;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Exceptions;
Expand All @@ -19,17 +24,20 @@ public class DevicesController : Controller
private readonly IDeviceService _deviceService;
private readonly IUserService _userService;
private readonly IUserRepository _userRepository;
private readonly ICurrentContext _currentContext;

public DevicesController(
IDeviceRepository deviceRepository,
IDeviceService deviceService,
IUserService userService,
IUserRepository userRepository)
IUserRepository userRepository,
ICurrentContext currentContext)
{
_deviceRepository = deviceRepository;
_deviceService = deviceService;
_userService = userService;
_userRepository = userRepository;
_currentContext = currentContext;
}

[HttpGet("{id}")]
Expand Down Expand Up @@ -117,6 +125,55 @@ public async Task<DeviceResponseModel> PutKeys(string identifier, [FromBody] Dev
return response;
}

[HttpPost("{identifier}/retrieve-keys")]
public async Task<ProtectedDeviceResponseModel> GetDeviceKeys(string identifier, [FromBody] SecretVerificationRequestModel model)
{
var user = await _userService.GetUserByPrincipalAsync(User);

if (user == null)
{
throw new UnauthorizedAccessException();
}

if (!await _userService.VerifySecretAsync(user, model.Secret))
{
await Task.Delay(2000);
throw new BadRequestException(string.Empty, "User verification failed.");
}

var device = await _deviceRepository.GetByIdentifierAsync(identifier, user.Id);

if (device == null)
{
throw new NotFoundException();
}

return new ProtectedDeviceResponseModel(device);
}

[HttpPost("update-trust")]
public async Task PostUpdateTrust([FromBody] UpdateDevicesTrustRequestModel model)
justindbaur marked this conversation as resolved.
Show resolved Hide resolved
{
var user = await _userService.GetUserByPrincipalAsync(User);

if (user == null)
{
throw new UnauthorizedAccessException();
}

if (!await _userService.VerifySecretAsync(user, model.Secret))
{
await Task.Delay(2000);
throw new BadRequestException(string.Empty, "User verification failed.");
}

await _deviceService.UpdateDevicesTrustAsync(
_currentContext.DeviceIdentifier,
user.Id,
model.CurrentDevice,
model.OtherDevices ?? Enumerable.Empty<OtherDeviceKeysUpdateRequestModel>());
}

[HttpPut("identifier/{identifier}/token")]
[HttpPost("identifier/{identifier}/token")]
public async Task PutToken(string identifier, [FromBody] DeviceTokenRequestModel model)
Expand Down
11 changes: 4 additions & 7 deletions src/Api/Models/Response/DeviceResponseModel.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Bit.Core.Entities;
using Bit.Core.Auth.Utilities;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Models.Api;

Expand All @@ -19,17 +20,13 @@ public DeviceResponseModel(Device device)
Type = device.Type;
Identifier = device.Identifier;
CreationDate = device.CreationDate;
EncryptedUserKey = device.EncryptedUserKey;
EncryptedPublicKey = device.EncryptedPublicKey;
EncryptedPrivateKey = device.EncryptedPrivateKey;
IsTrusted = device.IsTrusted();
}

public string Id { get; set; }
public string Name { get; set; }
public DeviceType Type { get; set; }
public string Identifier { get; set; }
public DateTime CreationDate { get; set; }
public string EncryptedUserKey { get; }
public string EncryptedPublicKey { get; }
public string EncryptedPrivateKey { get; }
public bool IsTrusted { get; set; }
}
21 changes: 21 additions & 0 deletions src/Core/Auth/Models/Api/Request/DeviceKeysUpdateRequestModel.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
using System.ComponentModel.DataAnnotations;
using Bit.Core.Utilities;

namespace Bit.Core.Auth.Models.Api.Request;

public class OtherDeviceKeysUpdateRequestModel : DeviceKeysUpdateRequestModel
{
[Required]
public Guid DeviceId { get; set; }
}

public class DeviceKeysUpdateRequestModel
{
[Required]
[EncryptedString]
public string EncryptedPublicKey { get; set; }

[Required]
[EncryptedString]
public string EncryptedUserKey { get; set; }
}
30 changes: 30 additions & 0 deletions src/Core/Auth/Models/Api/Response/ProtectedDeviceResponseModel.cs
justindbaur marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Models.Api;

namespace Bit.Core.Auth.Models.Api.Response;

public class ProtectedDeviceResponseModel : ResponseModel
{
public ProtectedDeviceResponseModel(Device device)
: base("protectedDevice")
{
ArgumentNullException.ThrowIfNull(device);

Id = device.Id;
Name = device.Name;
Type = device.Type;
Identifier = device.Identifier;
CreationDate = device.CreationDate;
EncryptedUserKey = device.EncryptedUserKey;
EncryptedPublicKey = device.EncryptedPublicKey;
}

public Guid Id { get; set; }
public string Name { get; set; }
public DeviceType Type { get; set; }
public string Identifier { get; set; }
public DateTime CreationDate { get; set; }
public string EncryptedUserKey { get; set; }
public string EncryptedPublicKey { get; set; }
}
7 changes: 6 additions & 1 deletion src/Core/Services/IDeviceService.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Bit.Core.Entities;
using Bit.Core.Auth.Models.Api.Request;
using Bit.Core.Entities;

namespace Bit.Core.Services;

Expand All @@ -7,4 +8,8 @@ public interface IDeviceService
Task SaveAsync(Device device);
Task ClearTokenAsync(Device device);
Task DeleteAsync(Device device);
Task UpdateDevicesTrustAsync(string currentDeviceIdentifier,
Guid currentUserId,
DeviceKeysUpdateRequestModel currentDeviceUpdate,
IEnumerable<OtherDeviceKeysUpdateRequestModel> alteredDevices);
}
62 changes: 61 additions & 1 deletion src/Core/Services/Implementations/DeviceService.cs
justindbaur marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
using Bit.Core.Entities;
using Bit.Core.Auth.Models.Api.Request;
using Bit.Core.Auth.Utilities;
using Bit.Core.Entities;
using Bit.Core.Exceptions;
using Bit.Core.Repositories;

namespace Bit.Core.Services;
Expand Down Expand Up @@ -43,4 +46,61 @@ public async Task DeleteAsync(Device device)
await _deviceRepository.DeleteAsync(device);
await _pushRegistrationService.DeleteRegistrationAsync(device.Id.ToString());
}

public async Task UpdateDevicesTrustAsync(string currentDeviceIdentifier,
Guid currentUserId,
DeviceKeysUpdateRequestModel currentDeviceUpdate,
IEnumerable<OtherDeviceKeysUpdateRequestModel> alteredDevices)
{
var existingDevices = await _deviceRepository.GetManyByUserIdAsync(currentUserId);

var currentDevice = existingDevices.FirstOrDefault(d => d.Identifier == currentDeviceIdentifier);

if (currentDevice == null)
{
throw new NotFoundException();
}

existingDevices.Remove(currentDevice);

var alterDeviceKeysDict = alteredDevices.ToDictionary(d => d.DeviceId);

if (alterDeviceKeysDict.ContainsKey(currentDevice.Id))
{
throw new BadRequestException("Current device can not be an optional rotation.");
}

currentDevice.EncryptedPublicKey = currentDeviceUpdate.EncryptedPublicKey;
currentDevice.EncryptedUserKey = currentDeviceUpdate.EncryptedUserKey;

await _deviceRepository.UpsertAsync(currentDevice);

foreach (var device in existingDevices)
{
if (!device.IsTrusted())
{
// You can't update the trust of a device that isn't trusted to begin with
// should we throw and consider this a BadRequest? If we want to consider it a invalid request
// we need to check that information before we enter this foreach, we don't want to partially complete
// this process.
continue;
}

if (alterDeviceKeysDict.TryGetValue(device.Id, out var updateRequest))
{
// An update to this device was requested
device.EncryptedPublicKey = updateRequest.EncryptedPublicKey;
device.EncryptedUserKey = updateRequest.EncryptedUserKey;
}
else
{
// No update to this device requested, just untrust it
device.EncryptedUserKey = null;
device.EncryptedPublicKey = null;
device.EncryptedPrivateKey = null;
}

await _deviceRepository.UpsertAsync(device);
}
}
}
63 changes: 62 additions & 1 deletion test/Core.Test/Services/DeviceServiceTests.cs
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
using Bit.Core.Entities;
using Bit.Core.Auth.Models.Api.Request;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Test.Common.AutoFixture;
using Bit.Test.Common.AutoFixture.Attributes;
using NSubstitute;
using Xunit;

namespace Bit.Core.Test.Services;

[SutProviderCustomize]
public class DeviceServiceTests
{
[Fact]
Expand All @@ -33,4 +37,61 @@ public async Task DeviceSaveShouldUpdateRevisionDateAndPushRegistration()
await pushRepo.Received().CreateOrUpdateRegistrationAsync("testtoken", id.ToString(),
userId.ToString(), "testid", DeviceType.Android);
}

/// <summary>
///
/// </summary>
[Theory, BitAutoData]
public async Task UpdateDevicesTrustAsync_Works(
SutProvider<DeviceService> sutProvider,
Guid currentUserId,
Device deviceOne,
Device deviceTwo,
justindbaur marked this conversation as resolved.
Show resolved Hide resolved
Device deviceThree)
{
deviceOne.Identifier = "current_device";

sutProvider.GetDependency<IDeviceRepository>()
.GetManyByUserIdAsync(currentUserId)
.Returns(new List<Device>
{
deviceOne,
deviceTwo,
deviceThree,
});

var currentDeviceModel = new DeviceKeysUpdateRequestModel
{
EncryptedPublicKey = "current_encrypted_public_key",
EncryptedUserKey = "current_encrypted_user_key",
};

var alteredDeviceModels = new List<OtherDeviceKeysUpdateRequestModel>
{
new OtherDeviceKeysUpdateRequestModel
{
DeviceId = deviceTwo.Id,
EncryptedPublicKey = "encrypted_public_key_two",
EncryptedUserKey = "encrypted_user_key_two",
},
};

await sutProvider.Sut.UpdateDevicesTrustAsync("current_device", currentUserId, currentDeviceModel, alteredDeviceModels);

await sutProvider.GetDependency<IDeviceRepository>()
.Received(1)
.UpsertAsync(Arg.Is<Device>(d => d.Id == deviceOne.Id && d.EncryptedPublicKey == "current_encrypted_public_key"));
justindbaur marked this conversation as resolved.
Show resolved Hide resolved

await sutProvider.GetDependency<IDeviceRepository>()
.Received(1)
.UpsertAsync(Arg.Is<Device>(d => d.Id == deviceTwo.Id && d.EncryptedPublicKey == "encrypted_public_key_two"));

await sutProvider.GetDependency<IDeviceRepository>()
.Received(1)
.UpsertAsync(Arg.Is<Device>(d => d.Id == deviceThree.Id && d.EncryptedPublicKey == null));

await sutProvider.GetDependency<IDeviceRepository>()
.Received(3)
.UpsertAsync(Arg.Any<Device>());
}
}
justindbaur marked this conversation as resolved.
Show resolved Hide resolved