AGD-1300 Add mechanism for common package directory similar to common definition directory alternative#10208
Conversation
…ctory to PathManager
… before adding the package
…pkg.json manifest
…f they are found in directories requiring verification
| } | ||
| catch | ||
| { | ||
| throw new Exception(String.Format( |
There was a problem hiding this comment.
do these exceptions end up in the console only or are they more visible to the user?
There was a problem hiding this comment.
I don't know yet. I will test and verify
| var cert = asm.Modules.FirstOrDefault()?.GetSignerCertificate(); | ||
| if (cert != null) | ||
| { | ||
| var cert2 = new System.Security.Cryptography.X509Certificates.X509Certificate2(cert); |
There was a problem hiding this comment.
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?
| var cert = asm.Modules.FirstOrDefault()?.GetSignerCertificate(); | ||
| if (cert != null) | ||
| { | ||
| var cert2 = new System.Security.Cryptography.X509Certificates.X509Certificate2(cert); |
There was a problem hiding this comment.
this code seems exactly the same - what am I missing? can't we use the version in dynamo core?
There was a problem hiding this comment.
Not sure what you mean? That there is similar code here and in the package loading path?
There was a problem hiding this comment.
yes, there's a bunch of duplicate code here that can get out of sync.
src/DynamoPackages/PackageLoader.cs
Outdated
| discoveredPkg.Name, discoveredPkg.RootDirectory)); | ||
| } | ||
|
|
||
| //Verify teh node libarary has a verified signed certificate |
src/DynamoPackages/PackageLoader.cs
Outdated
| discoveredPkg.Name, discoveredPkg.RootDirectory)); | ||
| } | ||
|
|
||
| discoveredPkg.HasSignedEntryPoints = true; |
There was a problem hiding this comment.
what conditions will make this true?
There was a problem hiding this comment.
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
src/DynamoPackages/PackageLoader.cs
Outdated
| InitializePackageDirectories(packagesDirectories); | ||
| } | ||
|
|
||
| public PackageLoader(IEnumerable<string> packagesDirectories, IEnumerable<string> packageDirectoriesToVerify) |
There was a problem hiding this comment.
please add a summary for this constructor
src/DynamoPackages/PackageLoader.cs
Outdated
| InitializePackageDirectories(packagesDirectories); | ||
|
|
||
| if (packageDirectoriesToVerify == null) | ||
| throw new ArgumentNullException("packageDirectoriesToVerify"); |
There was a problem hiding this comment.
Not sure what you mean
There was a problem hiding this comment.
use nameof(packageDirectoriesToVerify)
src/DynamoPackages/PackageLoader.cs
Outdated
| return null; | ||
| } | ||
|
|
||
| private static void CheckPackageNodeLibraryCertificates(string directory, Package discoveredPkg) |
There was a problem hiding this comment.
this is a giant method - please add a summary of what it does.
src/DynamoPackages/Package.cs
Outdated
| return assemblies; | ||
| } | ||
|
|
||
| private static bool IsFileInManifestNodeLibraries(IEnumerable<string> nodeLibraries, string filename, string path) |
There was a problem hiding this comment.
I don't understand the name of this function.
src/DynamoPackages/Package.cs
Outdated
| } | ||
| } | ||
| catch | ||
| {; |
src/DynamoPackages/Package.cs
Outdated
|
|
||
| private static bool IsFileInManifestNodeLibraries(IEnumerable<string> nodeLibraries, string filename, string path) | ||
| { | ||
| foreach (var manifestFile in nodeLibraries) |
There was a problem hiding this comment.
this name (manifestFile) is confusing to me - what about nodeLibraryAssemblyName?
src/DynamoPackages/PackageLoader.cs
Outdated
| discoveredPkg.Name, discoveredPkg.RootDirectory)); | ||
| } | ||
|
|
||
| foreach (var manifestFile in discoveredPkg.Header.node_libraries) |
There was a problem hiding this comment.
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
aparajit-pratap
left a comment
aparajit-pratap
left a comment
There was a problem hiding this comment.
@saintentropy could these changes prevent existing packages and extensions to stop loading if they fail the signed certificate checks?
src/DynamoPackages/Package.cs
Outdated
| return assemblies; | ||
| } | ||
|
|
||
| private static bool IsFileInManifestNodeLibraries(IEnumerable<string> nodeLibraries, string filename, string path) |
There was a problem hiding this comment.
Does this check for the pkg.json file? Should it not be more appropriately named as IsManifestFileInNodeLibraries?
|
|
||
| internal List<string> directoriesToVerifyCertificates = new List<string>(); | ||
|
|
||
| private static bool CheckExtensionCertificates(ExtensionDefinition viewExtension) |
There was a problem hiding this comment.
I think the parameter name should simply be extension.
| //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)); | ||
| } |
There was a problem hiding this comment.
These checks don't seem specific to checking certificates. Can they be moved outside this method?
| Assembly asm; | ||
| try | ||
| { | ||
| asm = Assembly.ReflectionOnlyLoadFrom(viewExtension.AssemblyPath); |
There was a problem hiding this comment.
Could you explain why is this check sufficient? Aren't the assembly methods loaded not only for reflection but also for execution?
| viewExtension.TypeName, viewExtension.AssemblyPath)); | ||
| } | ||
|
|
||
| //Verify teh node libarary has a verified signed certificate |
| } | ||
|
|
||
| throw new Exception(String.Format( | ||
| "A view extension called {0} found at {1} did not have a signed certificate. Ignoring it.", |
There was a problem hiding this comment.
Add to string resources.
| { | ||
| 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)); |
There was a problem hiding this comment.
Add to resx files.
| { | ||
| 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)); |
There was a problem hiding this comment.
Must be string resource.
|
|
||
| throw new Exception(String.Format( | ||
| "An extension called {0} found at {1} did not have a signed certificate. Ignoring it.", | ||
| viewExtension.TypeName, viewExtension.AssemblyPath)); |
…nting to dll outside of check dir
| [DataMember] | ||
| public string TypeName { get; set; } | ||
|
|
||
| public bool RequiresSignedEntryPoint { get; internal set; } = false; |
There was a problem hiding this comment.
hmmmmm - does this effect existing extensions and their automatic serialization and deserialization?
Can you show a sample?
There was a problem hiding this comment.
doesn't this seem kind of fragile?... / easily defeat-able
There was a problem hiding this comment.
hmmm... Didn't realize we automatically deserialize. But yes that would be not good
There was a problem hiding this comment.
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
There was a problem hiding this comment.
hmm - I see - Wonder why the dataMember attributes were used.
So you don't intend for this property to be deserialized?
| [DataMember] | ||
| public string TypeName { get; set; } | ||
|
|
||
| public bool RequiresSignedEntryPoint { get; internal set; } = false; |
src/DynamoPackages/Package.cs
Outdated
| /// </summary> | ||
| public PackageUploadRequestBody Header { get; internal set; } | ||
|
|
||
| public bool RequiresSignedEntryPoints { get; internal set; } |
There was a problem hiding this comment.
does this need to be public? Does this effect the package header? Summary if it needs to be public.
There was a problem hiding this comment.
If it effects header then we need to consider if this effects package upload/download?
There was a problem hiding this comment.
Will move this to internal as well
|
@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... |
|
I guess I thought the sample packages were in the repo. I just signed the existing package sample |
… 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)
4 participants
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
*.resxfilesReviewers
@mjkkirschner @QilongTang
FYIs
@sm6srw @Dewb