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

Make UID optional in frame submission #618

Merged
merged 6 commits into from
Feb 24, 2020

Conversation

Porges
Copy link
Contributor

@Porges Porges commented Feb 3, 2020

A non-existing UID means to run the job as the RQD user on the render host; RQD does not attempt to switch users. This has security implications but probably not worse than the current setup which allows any UID to be specified, although I know some teams are running RQD as root...

This change allows Cuesubmit on Windows to submit jobs for Linux, as it will not send UIDs from Windows.

Changes:

  • Alter database to allow UID to be NULL (not done to Oracle as support is going to be dropped)
  • Alter DAO classes to understand missing UID.
  • Alter domain classes to allow UID to be Optional<Integer>.
  • Update CJSL DTD to allow UID to be optional.
  • Update .proto files to allow UID to be missing. According to Proto3 docs, this is binary-compatible. However, it is not backward-compatible at the application level. Old RQDs will see a missing UID as UID 0, and refuse to run the frame.
  • Update Cuesubmit to not send UIDs from Windows.

A non-existing UID means to run the job as the RQD user on the render
host; RQD does not attempt to switch users. (This has security
implications but probably not worse than the current setup which allows
any UID to be specified.)

This change allows Windows to submit jobs for Linux, as Cuesubmit will
now never send UIDs from Windows.

Changes:
- Alter database to allow UID to be `NULL` (not done to Oracle as
support is going to be dropped)
- Alter DAO classes to understand missing UID.
- Alter domain classes to allow UID to be `Optional<Integer>`.
- Update CJSL DTD to allow UID to be optional.
- Update .proto files to allow UID to be missing. According to Proto3
docs, this is binary-compatible. However, it is not backward-compatible
at the application level. Old RQDs will see a missing UID as UID 0
and refuse to run the frame.
- Update Cuesubmit to not send UIDs from Windows.
try: # Exception block for all exceptions

# Change to frame user if needed:
if runFrame.HasField("uid"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably also have a check here to ensure that if RQD is running as root then a uid must be provided. This would continue the current behaviour of disallowing running frames as root.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really strongly about enforcing such a requirement. Certainly switching to root by passing a UID should not be possible, but if a user wants to run RQD as root and runs jobs as root, I think that's their choice.

What happens in a Docker environment, i.e. if someone has built a container where that user appears as root to RQD?

Copy link
Collaborator

@bcipriano bcipriano left a comment

Choose a reason for hiding this comment

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

Overall this is looking good -- ready for final review or still working on it?

proto/job.proto Show resolved Hide resolved
@Porges
Copy link
Contributor Author

Porges commented Feb 5, 2020

I’ll need to update the migration version number assuming that #621 is merged first. Otherwise I’m happy.

However, there’s no migration in here for Oracle yet.

@bcipriano
Copy link
Collaborator

#621 is merged now, so you're good to pull those changes in and increment that version number.

@Porges
Copy link
Contributor Author

Porges commented Feb 17, 2020

Migration renamed.

Copy link
Collaborator

@bcipriano bcipriano left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

This looks ok without the Oracle migration to me? I mean if someone is running Oracle and tries to submit a job with UID=null it will fail, but our remaining Oracle user will almost certainly stick to the existing UID system.

Copy link
Collaborator

@gregdenton gregdenton left a comment

Choose a reason for hiding this comment

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

LGTM. I agree with @bcipriano that we should be good to submit this without an Oracle side. Maybe in the next release notes call out that the Oracle support is being deprecated?

@bcipriano
Copy link
Collaborator

Yeah, that sounds good.

@bcipriano bcipriano merged commit 3877834 into AcademySoftwareFoundation:master Feb 24, 2020
@Porges Porges deleted the optional-uid branch February 24, 2020 21: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.

None yet

4 participants