AGD-1300 Add mechanism for common package directory similar to common definition directory alternative #10208

saintentropy · 2019-12-09T15:31:23Z

Purpose

The purpose of this PR is to add a mechanism to allow a common package installation directory similar to the common custom node directly already supported in the PathManager. The common package directly will allow for the installation of packages that are available to all users on a specific machine. This common directory will supplement the current default user package and definitions directories located in user/appdata/...

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.

Reviewers

@mjkkirschner @QilongTang

FYIs

@sm6srw @Dewb

…ctory to PathManager

… before adding the package

…pkg.json manifest

…t to check

…f they are found in directories requiring verification

src/DynamoCore/Configuration/PathManager.cs

src/DynamoCore/Extensions/ExtensionLoader.cs

src/DynamoCoreWpf/Extensions/ViewExtensionLoader.cs

src/DynamoCore/Configuration/PathManager.cs

src/DynamoCore/Extensions/ExtensionLoader.cs

mjkkirschner · 2019-12-09T22:46:54Z

src/DynamoCore/Extensions/ExtensionLoader.cs

+            }
+            catch
+            {
+                throw new Exception(String.Format(


do these exceptions end up in the console only or are they more visible to the user?

I don't know yet. I will test and verify

mjkkirschner · 2019-12-09T22:47:52Z

src/DynamoCore/Extensions/ExtensionLoader.cs

+            var cert = asm.Modules.FirstOrDefault()?.GetSignerCertificate();
+            if (cert != null)
+            {
+                var cert2 = new System.Security.Cryptography.X509Certificates.X509Certificate2(cert);


have you verified this works on linux/mac under mono? .net Core? - if not I wonder if this flow should be disabled if running on non windows platform?

src/DynamoCore/Models/DynamoModel.cs

mjkkirschner · 2019-12-09T22:49:39Z

src/DynamoCoreWpf/Extensions/ViewExtensionLoader.cs

+            var cert = asm.Modules.FirstOrDefault()?.GetSignerCertificate();
+            if (cert != null)
+            {
+                var cert2 = new System.Security.Cryptography.X509Certificates.X509Certificate2(cert);


this code seems exactly the same - what am I missing? can't we use the version in dynamo core?

Not sure what you mean? That there is similar code here and in the package loading path?

yes, there's a bunch of duplicate code here that can get out of sync.

src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs

mjkkirschner · 2019-12-09T22:51:13Z

src/DynamoPackages/PackageLoader.cs

+                            discoveredPkg.Name, discoveredPkg.RootDirectory));
+                }
+
+                //Verify teh node libarary has a verified signed certificate


spelling the

mjkkirschner · 2019-12-09T22:51:41Z

src/DynamoPackages/PackageLoader.cs

+                        discoveredPkg.Name, discoveredPkg.RootDirectory));
+            }
+
+            discoveredPkg.HasSignedEntryPoints = true;


what conditions will make this true?

You get here when you have looped through each file in the node_libraries and verified it has a cert that is verified. Everywhere else you hit a exception

mjkkirschner · 2019-12-09T22:52:09Z

src/DynamoPackages/PackageLoader.cs

+            InitializePackageDirectories(packagesDirectories);
+        }
+
+        public PackageLoader(IEnumerable<string> packagesDirectories, IEnumerable<string> packageDirectoriesToVerify)


please add a summary for this constructor

mjkkirschner · 2019-12-09T22:52:25Z

src/DynamoPackages/PackageLoader.cs

+            InitializePackageDirectories(packagesDirectories);
+
+            if (packageDirectoriesToVerify == null)
+                throw new ArgumentNullException("packageDirectoriesToVerify");


Not sure what you mean

use nameof(packageDirectoriesToVerify)

mjkkirschner · 2019-12-09T22:54:11Z

src/DynamoPackages/PackageLoader.cs

            return null;
        }

+        private static void CheckPackageNodeLibraryCertificates(string directory, Package discoveredPkg)


this is a giant method - please add a summary of what it does.

mjkkirschner · 2019-12-09T22:57:36Z

src/DynamoPackages/Package.cs

            return assemblies;
        }

+        private static bool IsFileInManifestNodeLibraries(IEnumerable<string> nodeLibraries, string filename, string path)


I don't understand the name of this function.

mjkkirschner · 2019-12-09T22:58:21Z

src/DynamoPackages/Package.cs

+                    }
+                }
+                catch
+                {;


dangling semicolon?

mjkkirschner · 2019-12-09T23:00:17Z

src/DynamoPackages/Package.cs


+        private static bool IsFileInManifestNodeLibraries(IEnumerable<string> nodeLibraries, string filename, string path)
+        {
+            foreach (var manifestFile in nodeLibraries)


this name (manifestFile) is confusing to me - what about nodeLibraryAssemblyName?

mjkkirschner · 2019-12-09T23:03:05Z

src/DynamoPackages/PackageLoader.cs

+                        discoveredPkg.Name, discoveredPkg.RootDirectory));
+            }
+
+            foreach (var manifestFile in discoveredPkg.Header.node_libraries)


same with the name here - unless I am misunderstanding - personally I would get this confused with the manifest file itself - like the header file or the extensionManfiest

src/DynamoPackages/PackageLoader.cs

aparajit-pratap

@saintentropy could these changes prevent existing packages and extensions to stop loading if they fail the signed certificate checks?

aparajit-pratap · 2019-12-09T20:56:26Z

src/DynamoPackages/Package.cs

            return assemblies;
        }

+        private static bool IsFileInManifestNodeLibraries(IEnumerable<string> nodeLibraries, string filename, string path)


Does this check for the pkg.json file? Should it not be more appropriately named as IsManifestFileInNodeLibraries?

aparajit-pratap · 2019-12-09T21:28:38Z

src/DynamoCore/Extensions/ExtensionLoader.cs

+
+        internal List<string> directoriesToVerifyCertificates = new List<string>();
+
+        private static bool CheckExtensionCertificates(ExtensionDefinition viewExtension)


I think the parameter name should simply be extension.

aparajit-pratap · 2019-12-09T21:30:29Z

src/DynamoCore/Extensions/ExtensionLoader.cs

+            //Verify the node library exists in the package bin directory
+            if (!File.Exists(viewExtension.AssemblyPath))
+            {
+                throw new Exception(String.Format(
+                    "An extension called {0} found at {1} is missing dlls which are defined in the view extension definition.  Ignoring it.",
+                    viewExtension.TypeName, viewExtension.AssemblyPath));
+            }
+
+            //Verify that you can load the node library assembly into a Reflection only context
+            Assembly asm;
+            try
+            {
+                asm = Assembly.ReflectionOnlyLoadFrom(viewExtension.AssemblyPath);
+            }
+            catch
+            {
+                throw new Exception(String.Format(
+                    "An extension called {0} found at {1} has a dll which could not be loaded.  Ignoring it.",
+                    viewExtension.TypeName, viewExtension.AssemblyPath));
+            }


These checks don't seem specific to checking certificates. Can they be moved outside this method?

aparajit-pratap · 2019-12-09T21:32:10Z

src/DynamoCore/Extensions/ExtensionLoader.cs

+            Assembly asm;
+            try
+            {
+                asm = Assembly.ReflectionOnlyLoadFrom(viewExtension.AssemblyPath);


Could you explain why is this check sufficient? Aren't the assembly methods loaded not only for reflection but also for execution?

aparajit-pratap · 2019-12-09T21:32:40Z

src/DynamoCore/Extensions/ExtensionLoader.cs

+                    viewExtension.TypeName, viewExtension.AssemblyPath));
+            }
+
+            //Verify teh node libarary has a verified signed certificate


typo: the

src/DynamoPackages/PackageLoader.cs

aparajit-pratap · 2019-12-10T01:09:39Z

src/DynamoCoreWpf/Extensions/ViewExtensionLoader.cs

+            }
+
+            throw new Exception(String.Format(
+                "A view extension called {0} found at {1} did not have a signed certificate.  Ignoring it.",


Add to string resources.

aparajit-pratap · 2019-12-10T01:09:59Z

src/DynamoCoreWpf/Extensions/ViewExtensionLoader.cs

+            {
+                throw new Exception(String.Format(
+                    "A view extension called {0} found at {1} has a dll which could not be loaded.  Ignoring it.",
+                    viewExtension.TypeName, viewExtension.AssemblyPath));


Add to resx files.

aparajit-pratap · 2019-12-10T01:10:18Z

src/DynamoCoreWpf/Extensions/ViewExtensionLoader.cs

+            {
+                throw new Exception(String.Format(
+                        "A view extension called {0} found at {1} is missing dlls which are defined in the view extension definition.  Ignoring it.",
+                        viewExtension.TypeName, viewExtension.AssemblyPath));


Must be string resource.

aparajit-pratap · 2019-12-10T01:11:31Z

src/DynamoCore/Extensions/ExtensionLoader.cs

+
+            throw new Exception(String.Format(
+                "An extension called {0} found at {1} did not have a signed certificate.  Ignoring it.",
+                viewExtension.TypeName, viewExtension.AssemblyPath));


string resource

…nting to dll outside of check dir

mjkkirschner · 2019-12-11T20:40:17Z

src/DynamoCore/Extensions/ExtensionDefinition.cs

        [DataMember]
        public string TypeName { get; set; }
+
+        public bool RequiresSignedEntryPoint { get; internal set; } = false;


needs a summary

hmmmmm - does this effect existing extensions and their automatic serialization and deserialization?
Can you show a sample?

doesn't this seem kind of fragile?... / easily defeat-able

hmmm... Didn't realize we automatically deserialize. But yes that would be not good

We don't ever serialize or deserialize to this class automatically. We had build this class object always. But regardless, this does not have to be a public property. I will move it and the ViewExtension version to internal

hmm - I see - Wonder why the dataMember attributes were used.

So you don't intend for this property to be deserialized?

mjkkirschner · 2019-12-11T20:42:10Z

src/DynamoCoreWpf/Extensions/ViewExtensionDefinition.cs

        [DataMember]
        public string TypeName { get; set; }
+
+        public bool RequiresSignedEntryPoint { get; internal set; } = false;


comments etc

mjkkirschner · 2019-12-11T20:43:20Z

src/DynamoPackages/Package.cs

        /// </summary>
        public PackageUploadRequestBody Header { get; internal set; }

+        public bool RequiresSignedEntryPoints { get; internal set; }


does this need to be public? Does this effect the package header? Summary if it needs to be public.

If it effects header then we need to consider if this effects package upload/download?

Will move this to internal as well

mjkkirschner · 2019-12-12T03:47:35Z

@saintentropy the tests look good - but it would be useful to have the source for the dlls in the repo I guess... though I guess we can always decompile them...

saintentropy · 2019-12-12T03:56:49Z

I guess I thought the sample packages were in the repo. I just signed the existing package sample

QilongTang

LGTM

… definition directory alternative (DynamoDS#10208) * Add common package folder location similar to common definitions directory to PathManager * Initalize common package directory * Add a list of root directories to verify certificates and check those before adding the package * fix null value check * Update comments * Modify package and package loader to only check libraries defined in pkg.json manifest * Check view extension for valid signed certificate if path matches root to check * Upate extension loader to validate extension have valid certificate if they are found in directories requiring verification * Rename package property for to RequiresSignedEntryPoints * Fix bugs in package loader signed check * Revise view extension / extension loading logic to avoid ext deff pointing to dll outside of check dir * Comments * Package PR comments * Extension loading pr comments * View extension pr comments * On more comment * Refactor .Net assembly cetificate verication to DynamoCrypto * Consume validate method from DyanmoCrypto * Add comments * Move cert verification method to DynamoUtilities Class * Add new CertificateVerification class to DynamoUtilities * Update comments * Extract package error messages to resource files * Add comments and change RequiresSignedEntryPoints to internal * Add test Cases for signed packages (cherry picked from commit 717bb16)

* AGD-1300 Add mechanism for common package directory similar to common definition directory alternative (#10208) * Add common package folder location similar to common definitions directory to PathManager * Initalize common package directory * Add a list of root directories to verify certificates and check those before adding the package * fix null value check * Update comments * Modify package and package loader to only check libraries defined in pkg.json manifest * Check view extension for valid signed certificate if path matches root to check * Upate extension loader to validate extension have valid certificate if they are found in directories requiring verification * Rename package property for to RequiresSignedEntryPoints * Fix bugs in package loader signed check * Revise view extension / extension loading logic to avoid ext deff pointing to dll outside of check dir * Comments * Package PR comments * Extension loading pr comments * View extension pr comments * On more comment * Refactor .Net assembly cetificate verication to DynamoCrypto * Consume validate method from DyanmoCrypto * Add comments * Move cert verification method to DynamoUtilities Class * Add new CertificateVerification class to DynamoUtilities * Update comments * Extract package error messages to resource files * Add comments and change RequiresSignedEntryPoints to internal * Add test Cases for signed packages (cherry picked from commit 717bb16) * AGD-1300 Test Followup (#10211) * Add untitlity test (cherry picked from commit 2fcd3b5) * Fix lib tests (cherry picked from commit 7ae1302) * Add check to package property (cherry picked from commit 8e1bc3c) * Mark test for failure (cherry picked from commit 32d7a2d) * Agd-1300 Turn on valid package test (#10213) * Update the signed package file to uniqure namespace (cherry picked from commit 0005f38) * Add comment including the zt node (cherry picked from commit 16c28de) * Update comment (cherry picked from commit 75346ca)

saintentropy added 8 commits November 23, 2019 15:14

Add common package folder location similar to common definitions dire…

71f63dc

…ctory to PathManager

Initalize common package directory

fd6e00e

Add a list of root directories to verify certificates and check those…

8761df3

… before adding the package

fix null value check

0b2c065

Update comments

1bb47c6

Modify package and package loader to only check libraries defined in …

0381c8f

…pkg.json manifest

Check view extension for valid signed certificate if path matches roo…

fa17dd2

…t to check

Upate extension loader to validate extension have valid certificate i…

48c1ef2

…f they are found in directories requiring verification

mjkkirschner reviewed Dec 9, 2019

View reviewed changes

src/DynamoCore/Configuration/PathManager.cs Show resolved Hide resolved

saintentropy mentioned this pull request Dec 9, 2019

AGD-1300 Add mechanism for common package directory similar to common definition directory #10165

Closed

7 tasks

QilongTang reviewed Dec 9, 2019

View reviewed changes

src/DynamoCore/Extensions/ExtensionLoader.cs Outdated Show resolved Hide resolved

QilongTang reviewed Dec 9, 2019

View reviewed changes

src/DynamoCore/Extensions/ExtensionLoader.cs Outdated Show resolved Hide resolved

QilongTang reviewed Dec 9, 2019

View reviewed changes

src/DynamoCoreWpf/Extensions/ViewExtensionLoader.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Dec 9, 2019

View reviewed changes

src/DynamoCore/Configuration/PathManager.cs Show resolved Hide resolved

mjkkirschner reviewed Dec 9, 2019

View reviewed changes

src/DynamoCore/Extensions/ExtensionLoader.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Dec 9, 2019

View reviewed changes

src/DynamoCore/Models/DynamoModel.cs Show resolved Hide resolved

mjkkirschner reviewed Dec 9, 2019

View reviewed changes

src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs Show resolved Hide resolved

mjkkirschner reviewed Dec 9, 2019

View reviewed changes

src/DynamoPackages/Package.cs Outdated

}

}

catch

{;

Copy link

Member

mjkkirschner Dec 9, 2019

Choose a reason for hiding this comment

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

dangling semicolon?

mjkkirschner reviewed Dec 9, 2019

View reviewed changes

src/DynamoPackages/PackageLoader.cs Show resolved Hide resolved

aparajit-pratap requested changes Dec 10, 2019

View reviewed changes

saintentropy added 15 commits December 9, 2019 20:23

Rename package property for to RequiresSignedEntryPoints

a9d2509

Fix bugs in package loader signed check

fccd58f

Revise view extension / extension loading logic to avoid ext deff poi…

72ac04c

…nting to dll outside of check dir

Comments

f040b1b

Package PR comments

a522068

Extension loading pr comments

2735fbb

View extension pr comments

63cde91

On more comment

8916979

Refactor .Net assembly cetificate verication to DynamoCrypto

2cdbca7

Consume validate method from DyanmoCrypto

dee0e61

Add comments

e11799d

Move cert verification method to DynamoUtilities Class

cd703ec

Add new CertificateVerification class to DynamoUtilities

ae263a7

Update comments

4a6962c

Extract package error messages to resource files

f6f4f94

mjkkirschner reviewed Dec 11, 2019

View reviewed changes

saintentropy added 2 commits December 11, 2019 15:57

Add comments and change RequiresSignedEntryPoints to internal

e4ec678

Add test Cases for signed packages

182399a

mjkkirschner added the LGTM Looks good to me label Dec 12, 2019

mjkkirschner approved these changes Dec 12, 2019

View reviewed changes

QilongTang approved these changes Dec 12, 2019

View reviewed changes

QilongTang merged commit 717bb16 into DynamoDS:master Dec 12, 2019

QilongTang added this to the 2.5.0 milestone Dec 13, 2019


		internal List<string> directoriesToVerifyCertificates = new List<string>();

		private static bool CheckExtensionCertificates(ExtensionDefinition viewExtension)

AGD-1300 Add mechanism for common package directory similar to common definition directory alternative #10208

AGD-1300 Add mechanism for common package directory similar to common definition directory alternative #10208

Uh oh!

Conversation

saintentropy commented Dec 9, 2019

Purpose

Declarations

Reviewers

FYIs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aparajit-pratap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mjkkirschner Dec 10, 2019 •

edited

Loading

mjkkirschner Dec 9, 2019 •

edited

Loading

mjkkirschner Dec 11, 2019 •

edited

Loading