Better errors from Download: Make GetFileSasUrl nullable (#3229)

This allows us to generate 404s when someone attempts to download from a non-existent container. At the moment we generate a 500 which isn't useful, or very good looks-wise.
This commit is contained in:
George Pollard 2023-06-28 10:24:32 +12:00 committed by GitHub
parent 4b437533f0
commit 7c0dc7b65e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 107 additions and 26 deletions

View File

@ -1,4 +1,5 @@
using System.Web;
using System.Net;
using System.Web;
using Azure.Storage.Sas;
using Microsoft.Azure.Functions.Worker;
using Microsoft.Azure.Functions.Worker.Http;
@ -45,6 +46,16 @@ public class Download {
BlobSasPermissions.Read,
TimeSpan.FromMinutes(5));
if (sasUri is null) {
// Note that we do not validate the existence of the file, only the container.
return await _context.RequestHandling.NotOk(req,
Error.Create(
ErrorCode.INVALID_CONTAINER,
"container not found"),
"generating download file SAS",
HttpStatusCode.NotFound);
}
return RequestHandling.Redirect(req, sasUri);
}
}

View File

@ -20,7 +20,7 @@ public interface IContainers {
public Async.Task<BlobContainerClient?> FindContainer(Container container, StorageType storageType);
public Async.Task<Uri> GetFileSasUrl(Container container, string name, StorageType storageType, BlobSasPermissions permissions, TimeSpan? duration = null);
public Async.Task<Uri?> GetFileSasUrl(Container container, string name, StorageType storageType, BlobSasPermissions permissions, TimeSpan? duration = null);
public Async.Task SaveBlob(Container container, string name, string data, StorageType storageType);
public Async.Task<Guid> GetInstanceId();
@ -145,8 +145,12 @@ public class Containers : IContainers {
return null;
}
public async Async.Task<Uri> GetFileSasUrl(Container container, string name, StorageType storageType, BlobSasPermissions permissions, TimeSpan? duration = null) {
var client = await FindContainer(container, storageType) ?? throw new Exception($"unable to find container: {container} - {storageType}");
public async Async.Task<Uri?> GetFileSasUrl(Container container, string name, StorageType storageType, BlobSasPermissions permissions, TimeSpan? duration = null) {
var client = await FindContainer(container, storageType);
if (client is null) {
return null;
}
var blobClient = client.GetBlobClient(name);
var timeWindow = SasTimeWindow(duration ?? TimeSpan.FromDays(30));
return _storage.GenerateBlobSasUri(permissions, blobClient, timeWindow);

View File

@ -114,9 +114,16 @@ namespace Microsoft.OneFuzz.Service {
return eventMessageResult.ErrorV;
}
var sasUrl = await _containers.GetFileSasUrl(WellKnownContainers.Events, eventId.ToString(), StorageType.Corpus, BlobSasPermissions.Read);
var sasUrl = await _containers.GetFileSasUrl(
WellKnownContainers.Events,
eventId.ToString(),
StorageType.Corpus,
BlobSasPermissions.Read);
if (sasUrl == null) {
return OneFuzzResult<DownloadableEventMessage>.Error(ErrorCode.UNABLE_TO_FIND, $"Could not find container for event with id {eventId}");
return OneFuzzResult<DownloadableEventMessage>.Error(
ErrorCode.UNABLE_TO_FIND,
$"Could not find container for event with id {eventId}");
}
var eventMessage = eventMessageResult.OkV!;
@ -133,8 +140,19 @@ namespace Microsoft.OneFuzz.Service {
}
public async Task<DownloadableEventMessage> MakeDownloadable(EventMessage eventMessage) {
await _containers.SaveBlob(WellKnownContainers.Events, eventMessage.EventId.ToString(), JsonSerializer.Serialize(eventMessage, _options), StorageType.Corpus);
var sasUrl = await _containers.GetFileSasUrl(WellKnownContainers.Events, eventMessage.EventId.ToString(), StorageType.Corpus, BlobSasPermissions.Read);
await _containers.SaveBlob(
WellKnownContainers.Events,
eventMessage.EventId.ToString(),
JsonSerializer.Serialize(eventMessage, _options),
StorageType.Corpus);
var sasUrl = await _containers.GetFileSasUrl(
WellKnownContainers.Events,
eventMessage.EventId.ToString(),
StorageType.Corpus,
BlobSasPermissions.Read)
// events container should always exist
?? throw new InvalidOperationException("Events container is missing");
return new DownloadableEventMessage(
eventMessage.EventId,

View File

@ -1,4 +1,5 @@
using System.Text.Json;
using System.IO;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Threading.Tasks;
using Azure.Core;
@ -374,7 +375,13 @@ public class Extensions : IExtensions {
return extensions.Select(extension => extension.GetAsVirtualMachineScaleSetExtension()).ToList();
}
public async Task<Dictionary<string, VirtualMachineExtensionData>> ReproExtensions(AzureLocation region, Os reproOs, Guid reproId, ReproConfig reproConfig, Container? setupContainer) {
public async Task<Dictionary<string, VirtualMachineExtensionData>> ReproExtensions(
AzureLocation region,
Os reproOs,
Guid reproId,
ReproConfig reproConfig,
Container? setupContainer) {
// TODO: what about contents of repro.ps1 / repro.sh?
var report = await _context.Reports.GetReport(reproConfig.Container, reproConfig.Path);
var checkedReport = report.EnsureNotNull($"invalid report: {reproConfig}");
@ -399,13 +406,13 @@ public class Extensions : IExtensions {
reproConfig.Path,
StorageType.Corpus,
BlobSasPermissions.Read
),
) ?? throw new InvalidDataException($"failed to get repro config url: container '{reproConfig.Container}' missing"),
await _context.Containers.GetFileSasUrl(
inputBlob.Container,
inputBlob.Name,
StorageType.Corpus,
BlobSasPermissions.Read
)
) ?? throw new InvalidDataException($"failed to get input blob url: container '{inputBlob.Container}' missing"),
};
List<string> reproFiles;
@ -437,21 +444,22 @@ public class Extensions : IExtensions {
);
foreach (var reproFile in reproFiles) {
urls.AddRange(new List<Uri>()
{
urls.Add(
await _context.Containers.GetFileSasUrl(
WellKnownContainers.ReproScripts,
reproFile,
StorageType.Config,
BlobSasPermissions.Read
),
await _context.Containers.GetFileSasUrl(
) // this container should always exist
?? throw new InvalidOperationException("repro scripts container missing"));
urls.Add(await _context.Containers.GetFileSasUrl(
WellKnownContainers.TaskConfigs,
$"{reproId}/{scriptName}",
StorageType.Config,
BlobSasPermissions.Read
)
});
) // this container should always exist
?? throw new InvalidOperationException("task configs container missing"));
}
var baseExtension = await AgentConfig(region, reproOs, AgentMode.Repro, urls: urls, withSas: true);
@ -472,13 +480,17 @@ public class Extensions : IExtensions {
WellKnownContainers.ProxyConfigs,
$"{region}/{proxyId}/config.json",
StorageType.Config,
BlobSasPermissions.Read);
BlobSasPermissions.Read)
// this should always exist
?? throw new InvalidOperationException("proxy config container missing");
var proxyManager = await _context.Containers.GetFileSasUrl(
WellKnownContainers.Tools,
$"linux/onefuzz-proxy-manager",
StorageType.Config,
BlobSasPermissions.Read);
BlobSasPermissions.Read)
// this should always exist
?? throw new InvalidOperationException("tools container missing");
var baseExtension =
await AgentConfig(region, Os.Linux, AgentMode.Proxy, new List<Uri> { config, proxyManager }, true);

View File

@ -45,7 +45,19 @@ public class NotificationOperations : Orm<Notification>, INotificationOperations
await foreach (var (task, containers) in GetQueueTasks()) {
if (containers.Contains(container)) {
_logTracer.LogInformation("queuing input {Container} {Filename} {TaskId}", container, filename, task.TaskId);
var url = await _context.Containers.GetFileSasUrl(container, filename, StorageType.Corpus, BlobSasPermissions.Read | BlobSasPermissions.Delete);
var url = await _context.Containers.GetFileSasUrl(
container,
filename,
StorageType.Corpus,
BlobSasPermissions.Read | BlobSasPermissions.Delete);
if (url is null) {
_logTracer.LogError("unable to generate sas url for missing container '{Container}'", container);
// try again later
throw new InvalidOperationException($"container '{container}' is missing");
}
await _context.Queue.SendMessage(task.TaskId.ToString(), url.ToString(), StorageType.Corpus);
}
}
@ -93,9 +105,9 @@ public class NotificationOperations : Orm<Notification>, INotificationOperations
public IAsyncEnumerable<(Task, IEnumerable<Container>)> GetQueueTasks() {
// Nullability mismatch: We filter tuples where the containers are null
return _context.TaskOperations.SearchStates(states: TaskStateHelper.AvailableStates)
.Select(task => (task, _context.TaskOperations.GetInputContainerQueues(task.Config)))
.Where(taskTuple => taskTuple.Item2.IsOk && taskTuple.Item2.OkV != null)
.Select(x => (Task: x.Item1, Containers: x.Item2.OkV))!;
.Select(task => (task, containers: _context.TaskOperations.GetInputContainerQueues(task.Config)))
.Where(x => x.containers.IsOk && x.containers.OkV is not null)
.Select(x => (Task: x.task, Containers: x.containers.OkV!));
}
public async Async.Task<OneFuzzResult<Notification>> Create(Container container, NotificationTemplate config, bool replaceExisting) {

View File

@ -130,8 +130,16 @@ public class ProxyOperations : StatefulOrm<Proxy, VmState, ProxyOperations>, IPr
public async Async.Task SaveProxyConfig(Proxy proxy) {
var forwards = await GetForwards(proxy);
var url = (await _context.Containers.GetFileSasUrl(WellKnownContainers.ProxyConfigs, $"{proxy.Region}/{proxy.ProxyId}/config.json", StorageType.Config, BlobSasPermissions.Read)).EnsureNotNull("Can't generate file sas");
var queueSas = await _context.Queue.GetQueueSas("proxy", StorageType.Config, QueueSasPermissions.Add).EnsureNotNull("can't generate queue sas") ?? throw new Exception("Queue sas is null");
var url = (await _context.Containers.GetFileSasUrl(
WellKnownContainers.ProxyConfigs,
$"{proxy.Region}/{proxy.ProxyId}/config.json",
StorageType.Config,
BlobSasPermissions.Read)).EnsureNotNull("proxy configs container missing");
var queueSas = await _context.Queue.GetQueueSas(
"proxy",
StorageType.Config,
QueueSasPermissions.Add).EnsureNotNull("can't generate queue sas") ?? throw new Exception("Queue sas is null");
var proxyConfig = new ProxyConfig(
Url: url,

View File

@ -55,6 +55,22 @@ public abstract class DownloadTestBase : FunctionTestBase {
Assert.Equal(ErrorCode.INVALID_REQUEST.ToString(), err.Title);
}
[Fact]
public async Async.Task Container_NotFound_Generates404() {
var req = TestHttpRequestData.Empty("GET");
// this container won't exist because we haven't explicitly created it
var url = new UriBuilder(req.Url) { Query = "container=xxx&filename=yyy" }.Uri;
req.SetUrl(url);
var func = new Download(Context);
var result = await func.Run(req);
Assert.Equal(HttpStatusCode.NotFound, result.StatusCode);
var err = BodyAs<ProblemDetails>(result);
Assert.Equal(ErrorCode.INVALID_CONTAINER.ToString(), err.Title);
}
[Fact]
public async Async.Task Download_RedirectsToResult_WithLocationHeader() {
// set up a file to download