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

DYN-3320: Add basic standard library support #11383

Merged
merged 4 commits into from
Jan 8, 2021
Merged

DYN-3320: Add basic standard library support #11383

merged 4 commits into from
Jan 8, 2021

Conversation

sm6srw
Copy link
Contributor

@sm6srw sm6srw commented Jan 6, 2021

Purpose

Packages should be loadable/shippable with core runtime zip and daily builds.

This pull request does:

  • introduce a new Standard Library\Packages folder that lives in the same folder as the DynamoPackages.dll
  • add this folder as a folder to load packages from in each PackageLoader constructor.
  • require that packages loaded from this location is signed.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@sm6srw sm6srw added the WIP label Jan 6, 2021
@sm6srw sm6srw removed the WIP label Jan 7, 2021
var error = PathHelper.CreateFolderIfNotExist(DefaultPackagesDirectory);

if (error != null)
Log(error);

packagesDirectoriesToVerifyCertificates.Add(StandardLibraryDirectory);
Copy link
Member

Choose a reason for hiding this comment

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

so I'm curious about this decision?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mjkkirschner do you mean packages that we own do not really need to be signed?

Copy link
Member

@mjkkirschner mjkkirschner Jan 7, 2021

Choose a reason for hiding this comment

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

I'm not sure, I want to discuss it, if other teams will use this loading mechanism then it's an additional cost for them, also they may load custom nodes from this location - I'm not sure if our currrent cert checks actually check all files or only .dlls - I'm also not sure if our build steps sign all files or only .dlls

Definitely we should consider the security of this loading location given that this package loading location is local to dynamo and so could be anywhere - not in a protected folder like %appData% or %programData% that requires user or admin permissions.

In addition, if we want to "protect" this loading location perhaps there are better ways - or additional mechanisms.

Copy link
Member

Choose a reason for hiding this comment

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

another thought - we may not want anything but ADSK packages to be loaded from here - if that was the case how would we enforce it?

Copy link
Contributor

Choose a reason for hiding this comment

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

These points bring another question to my mind - will we be telling integrators that they could save their product-specific packages to this loading location if they want those packages to be loaded at startup or will we give them an option to add custom package paths in addition to this one?

Copy link
Member

Choose a reason for hiding this comment

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

I imagined we would reuse this location - but I guess ultimately that depends what type of restrictions we want to put on this location, and if those same restrictions are acceptable/sensible for both use cases. Off the top of my head it seems to make sense to use the same set of rules because they'll have the same security concerns and many overlapping requirements - and if we can simplify, why not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, these are binaries that we ship and they should always be signed. That was my thinking. And dynamo binaries will always live in a protected location when an integrator use it because they will install Dynamo in some way or another. So they need to sign any binaries they add to this location regardless.

This needs more discussions.

Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

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

approving for now, because I think discussion regarding cert checks is likely out of scope and we can schedule an internal discussion and iterate on it with future tasks.

@sm6srw sm6srw merged commit 7f7a95f into master Jan 8, 2021
@sm6srw sm6srw deleted the DYN-3320 branch January 8, 2021 15:24
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.

4 participants