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

Use EncryptedFilesSyncJob #400

Merged
merged 15 commits into from
Mar 31, 2025
Merged

Use EncryptedFilesSyncJob #400

merged 15 commits into from
Mar 31, 2025

Conversation

abuabraham-ttd
Copy link
Contributor

@abuabraham-ttd abuabraham-ttd commented Mar 25, 2025

Update to use EncryptedFilesSyncJob and not PrivateSiteRefresh for syncing encrypted files.
Handle metadata missing first level hash

UID2-5060

@@ -27,6 +28,7 @@ public class EncryptedSaltStoreWriter extends SaltStoreWriter implements StoreWr
private StoreScope scope;
private RotatingCloudEncryptionKeyProvider cloudEncryptionKeyProvider;
private Integer siteId;
private JsonObject metadata;

Choose a reason for hiding this comment

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

Maybe plaintextMetadata instead

@@ -105,8 +107,19 @@ protected List<RotatingSaltProvider.SaltSnapshot> getSnapshots(RotatingSaltProvi
return this.previousSeenSnapshots;
Copy link

@vishalegbert-ttd vishalegbert-ttd Mar 26, 2025

Choose a reason for hiding this comment

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

These two lines caused me a lot of confusion

In upload(Object data, JsonObject extraMeta), we already get a collection of saltSnapshots, presumably one old and one new. There is no point uploading twice

From the collection received in upload(), find the old snapshots, set it as a property, something like this.oldSnapshots

Call super.upload on the newest snapshot only

In getSnapshots, it should return the concat of this.oldSnapshots and data (which is the newest snapshot we just uploaded)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, no, in current implementation.

The execution flow is;

For snapshot in snapshots:
call super.upload()

So second snapshot writes metadata without including earlier snapshot. Hence we store previousSeenSnapshots.

Copy link
Contributor Author

@abuabraham-ttd abuabraham-ttd Mar 26, 2025

Choose a reason for hiding this comment

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

@vishalegbert-ttd "Call super.upload on the newest snapshot only"

Will mean; metadata.json will only have newest entry

Choose a reason for hiding this comment

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

@Override
    protected List<RotatingSaltProvider.SaltSnapshot> getSnapshots(RotatingSaltProvider.SaltSnapshot data){
        if (this.allSnapshots == null) {
            throw new IllegalStateException("Snapshots are not initialized. Please call upload() first.");
        }

        return this.allSnapshots;
    }

    @Override
    protected JsonObject getMetadata() throws Exception {
        if (this.allSnapshots == null) {
            throw new IllegalStateException("Extra metadata is null. Please call upload() first.");
        }
        
        return this.extraMetadata;
    }


    @Override
    public void upload(Object data, JsonObject extraMeta) throws Exception {
        List<RotatingSaltProvider.SaltSnapshot> snapshots = new ArrayList<>((Collection<RotatingSaltProvider.SaltSnapshot>) data);

        int size = snapshots.size();

        // Process first n-1 snapshots
        for (int i = 0; i < size - 1; i++) {
            RotatingSaltProvider.SaltSnapshot snapshot = snapshots.get(i);
            String location = getSaltSnapshotLocation(snapshot);
            super.uploadSaltsSnapshot(snapshot, location);
        }

        // Upload the last snapshot
        this.allSnapshots = snapshots;
        this.extraMetadata = extraMeta;
        super.upload(snapshots.get(size - 1));
    }```

Choose a reason for hiding this comment

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

Does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as discussed on call yesterday. I updated to what I had in mind, that includes modification to SaltStoreWriter . We can have a chat today and use either of this.

@abuabraham-ttd abuabraham-ttd force-pushed the abu-add-encryption-urls branch from 340cf0d to 651cb78 Compare March 26, 2025 20:08
@@ -14,14 +14,14 @@

public class SaltEncryptionJob extends Job {
private final Collection<OperatorKey> globalOperators;
private final Collection<RotatingSaltProvider.SaltSnapshot> saltEntries;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes to accept saltProvider and not just saltEntries.

We can then pass unencrypted saltEntries along with metadata to encryption process.

List<Integer> desiredPublicState = PublicSiteUtil.getPublicSaltSites(globalOperators);
multiScopeStoreWriter.uploadPublicWithEncryption(desiredPublicState, saltEntries, null);
multiScopeStoreWriter.uploadPublicWithEncryption(desiredPublicState, saltProvider.getSnapshots(), saltProvider.getMetadata());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

passing metadata in extraMeta argument to use this info while encrypting per site.

protected void refreshProvider() {
// we do not need to refresh the provider on encrypted writers
}

Copy link
Contributor Author

@abuabraham-ttd abuabraham-ttd Mar 27, 2025

Choose a reason for hiding this comment

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

re-organised the method to not have this noop

}

@Override
protected List<RotatingSaltProvider.SaltSnapshot> getSnapshots(RotatingSaltProvider.SaltSnapshot data){
Copy link
Contributor Author

@abuabraham-ttd abuabraham-ttd Mar 27, 2025

Choose a reason for hiding this comment

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

dont need this as well, as getSnapshots is handled in upload(Object data, JsonObject extraMeta)

}
@SuppressWarnings("unchecked")
List<RotatingSaltProvider.SaltSnapshot> snapshots = new ArrayList<>((Collection<RotatingSaltProvider.SaltSnapshot>) data);
this.buildAndUploadMetadata(extraMeta, this.uploadASnapshotsAndGetMetadata(snapshots));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gets all snapshots, calls buildAndUploadMetadata with extraMeta and snapshotmeta.

buildAndUploadMetadata works same way for both unencrypted and encrypted

@@ -43,7 +43,7 @@ public SaltStoreWriter(JsonObject config, RotatingSaltProvider provider, FileMan
this.versionGenerator = versionGenerator;
}

protected List<RotatingSaltProvider.SaltSnapshot> getSnapshots(RotatingSaltProvider.SaltSnapshot data){
private List<RotatingSaltProvider.SaltSnapshot> getSnapshots(RotatingSaltProvider.SaltSnapshot data){
if (provider.getSnapshots() == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed override

}

protected void buildAndUploadMetadata(JsonObject metadata, JsonArray snapshotsMetadata ) throws Exception{
final Instant now = Instant.now();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

constructs metadata and upload based on init metadata.

For unencrypted, it is provider.getMetadata();
For encrypted, it is extraMeta

List<RotatingSaltProvider.SaltSnapshot> snapshots = this.getSnapshots(data);

protected JsonArray uploadASnapshotsAndGetMetadata(List<RotatingSaltProvider.SaltSnapshot> snapshots) throws Exception {
final JsonArray snapshotsMetadata = new JsonArray();
Copy link
Contributor Author

@abuabraham-ttd abuabraham-ttd Mar 27, 2025

Choose a reason for hiding this comment

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

,split it to a function, no change in implementation , except making it return snapshotsMetadata

// refresh manually
refreshProvider();
}

protected void refreshProvider() throws Exception {
private void refreshProvider() throws Exception {
provider.loadContent();
Copy link
Contributor Author

@abuabraham-ttd abuabraham-ttd Mar 27, 2025

Choose a reason for hiding this comment

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

removed the noop override. It can be private

public void upload(RotatingSaltProvider.SaltSnapshot data) throws Exception {
JsonObject metadata = this.getMetadata();
List<RotatingSaltProvider.SaltSnapshot> snapshots = this.getSnapshots(data);
this.buildAndUploadMetadata(metadata, this.uploadSnapshotsAndGetMetadata(snapshots));

Choose a reason for hiding this comment

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

The original SaltStoreWrite code only uploaded the latest snapshot, now it will re-upload all of them..

Copy link
Contributor Author

@abuabraham-ttd abuabraham-ttd Mar 27, 2025

Choose a reason for hiding this comment

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

`
public void upload(RotatingSaltProvider.SaltSnapshot data) throws Exception {
final Instant now = Instant.now();
final long generated = now.getEpochSecond();

    JsonObject metadata = null;
    try {
        metadata = provider.getMetadata();
    } catch (CloudStorageException e) {
        if (e.getMessage().contains("The specified key does not exist")) {
            metadata = new JsonObject();
        } else {
            throw e;
        }
    }
    // bump up metadata version
    metadata.put("version", versionGenerator.getVersion());
    metadata.put("generated", generated);

    final JsonArray snapshotsMetadata = new JsonArray();
    metadata.put("salts", snapshotsMetadata);

    List<RotatingSaltProvider.SaltSnapshot> snapshots = this.getSnapshots(data);

    for (RotatingSaltProvider.SaltSnapshot snapshot : snapshots) {
        final String location = getSaltSnapshotLocation(snapshot);
        final JsonObject snapshotMetadata = new JsonObject();
        snapshotMetadata.put("effective", snapshot.getEffective().toEpochMilli());
        snapshotMetadata.put("expires", snapshot.getExpires().toEpochMilli());
        snapshotMetadata.put("location", location);
        snapshotMetadata.put("size", snapshot.getAllRotatingSalts().length);
        snapshotsMetadata.add(snapshotMetadata);

        uploadSaltsSnapshot(snapshot, location);
    }

    fileManager.uploadMetadata(metadata, "salts", new CloudPath(provider.getMetadataPath()));

    // refresh manually
    refreshProvider();
}`

how? Isn't the behavior the same? loop through snapshots and upload. 

Or did I miss anything? 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

snapshots = this.getSnapshots(data)

controls what snapshots are returned, and it only returns newestEffective. It isn't changed (if I understand it correctly)

Choose a reason for hiding this comment

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

Ah no sorry you're right

@abuabraham-ttd abuabraham-ttd force-pushed the abu-add-encryption-urls branch from 78700c1 to e5e0095 Compare March 31, 2025 19:10
@abuabraham-ttd abuabraham-ttd merged commit 098c800 into main Mar 31, 2025
2 of 3 checks passed
@abuabraham-ttd abuabraham-ttd deleted the abu-add-encryption-urls branch March 31, 2025 23:43
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.

2 participants