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

Prevent Public Resource Creation #233

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

vbhayden
Copy link
Contributor

From #226

The goal of this change is to allow server admins to prevent public creation of competencies, frameworks, etc. by requiring that those requests be associated with a logged-in user and presumably leave that enforcement to an OIDC provider.

Security Impact: This will hopefully harden CASS against fuzz attackers.
Presumptive Impact: Servers configured this way will require a slightly modified UI that will understand when their respective instance requires authorized users and disable their "Add New __" widgets when necessary. These changes aren't included here and will be added later.

@vbhayden
Copy link
Contributor Author

@Lomilar My initial stab is to just add a middleware block between the auth and skyrepo steps, but this ends up snagging internal user creation requests made by the auth step -- calling the global EcRepository instance when it detects a new user login.

https://github.com/cassproject/CASS/blob/master/src/main/server/shims/auth.js#L111

Currently the EcRepository seems to route its requests into the same /api endpoint as everything else, but I didn't notice any special headers / keys on those auto-generated requests to distinguish them from requests made by the client. Is there a way to validate those internal requests so that this block can ignore them?

Open to alternatives also.

@Lomilar
Copy link
Member

Lomilar commented Jul 13, 2022

There is a missing authentication method, which is the only real method, which is via signature sheets (can be present in headers or in a POST).

I'd really look at injecting something like this into the signature sheet validation checks, since all OIDC/PKI (client side certs)/JWT based authentication ultimately results in a signature sheet proxy so that data modifications have the correct fingerprints.

sigSheet[i] = signature;

To see where this translation occurs, look at
https://github.com/cassproject/CASS/blob/master/src/main/server/shims/auth.js

This puts shims in to convert authentication of all sorts into signature sheets, something we call a "securing proxy".

To put it explicitly:

  • If there's no signatures in the signature sheet, there should be a rejection.

@vbhayden
Copy link
Contributor Author

That was the first try, but could've sworn I was still logging empty sheets when the user was logged in.

Will try that next then.

@Lomilar Lomilar added the enhancement Desired behavior. label Jul 13, 2022
@Lomilar
Copy link
Member

Lomilar commented Jul 13, 2022

That may be -- gets and whatnot don't have signature sheets necessarily, but let's ram our faces into that roadblock and find a way through.

I think the intent is to provide something a bit more useful and nuanced than border security, which anyone deploying cass could put in.

@vbhayden
Copy link
Contributor Author

So I tried a few different approaches based on your initial suggestion.

  1. Inserting a block into the last part of signatureSheet call ended up still allowing resources to be created anonymously, but not viewed as the search did get blocked.
let blockPublicCreation = !!process.env.NO_PUBLIC;
if (blockPublicCreation && sigSheet.length == 0)
    error("Forbidden", 403);

this.ctx.put("signatureSheet", sigSheet);
return sigSheet;
  1. Adding a flag to the signatureSheet call to indicate whether the requester intended to create resources, but this ended up still being omitted if the validateSignature call was creating something new as it didn't need to validate ownership for those.

  2. Ignoring the signature sheet in ctx if the request expects to make resources, which does work for existing users but ends up with the same issue as before where new users cause things to get squirrely and error out.

@vbhayden
Copy link
Contributor Author

vbhayden commented Jul 15, 2022

About there, looking for a way to confirm that the new object created with the auth handler was creating using the internal EcPpk class

if (p == null)
{
    p = new EcPerson();
    p.addOwner(i.ppk.toPk());
    p.assignId(repo.selectedServerProxy == null ? repo.selectedServer : repo.selectedServerProxy,i.ppk.toPk().fingerprint());
    p.name = name;
    p.email = email;
    await repo.saveTo(p);
}

Namely, this modified part of the validateSignatures call:

var signatures = await ((signatureSheet).call(this));
let signaturesPresent = signatures.length > 0;
let signaturesRequired = !!process.env.NO_PUBLIC;

if (oldGet == null) {
    if (signaturesRequired && !signaturesPresent)
        error("Forbidden, no public etc.", 403);
    else
        return null;
}

My thought atm is that validateSignatures will need to accept an argument for the object being created, which would let us check its owner and ID as being correctly from the EcPpk class in the cass library.

@vbhayden
Copy link
Contributor Author

@Lomilar looking through the new user object and the signature workflow, I'm not seeing a way to verify the first-time user's EcRepo.saveTo() call with signatures as it only has a public key initially -- with the signature sheet being generated afterwards for the ID manager

    await repo.saveTo(p);
}

let host = repo.selectedServerProxy == null ? repo.selectedServer : repo.selectedServerProxy;
let signatureSheet = await eim.signatureSheet(60000, host );

req.headers.signatureSheet = signatureSheet;
req.eim = eim;

Is there any other way to verify the authenticity of this initial Create Person job?

@vbhayden
Copy link
Contributor Author

vbhayden commented Jul 15, 2022

It looks like there's an eim overload for the saveTo function which does create a signature for the request at least, will look at that.

edit: looks like that did the trick -- may have unintended consequenes though if it's expecting to always use the default EcIdentityManager instance?

@vbhayden vbhayden marked this pull request as ready for review July 15, 2022 20:11
@vbhayden
Copy link
Contributor Author

Will be testing this on a live instance today to see if anything weird comes up.

@vbhayden
Copy link
Contributor Author

So after a day or so and adjusting the changes to be a bit more precise, it seems to be behaving without any issues:

  • Resource creation requires a logged-in user, giving a 403 otherwise
  • Previously-existing anonymous resources can still be modified / deleted by anyone
  • Only owners can modify their own resources

@Lomilar for the UI portion, the least invasive option might be exposing some sort of /api/about on the server that would return a set of feature properties -- with the first (and only atm) being blockPublicCreation. The Vue components would then be rebound and enabled / disabled depending on the combination of there being a known user and the server blocking public additions.

We're a bit pressed this week but I will work the UI side of this PR if the /about approach seems fine.

Making the No-Public aligned with main
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Desired behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants