Feature: Security Policies as Plugins #1003

mlgiraud · 2017-03-31T11:01:27Z

This PR implements a plugin architecture to support encryption with different security policies as plugins.
The architecture is mainly finished. Mainly error handling is still incomplete and im working on that.
Some encryption stuff still needs some work since im not quite sure about what the standard specifies.

I'm not quite done working on this, but thought it might be useful to already create a PR so others can see the current status. Also errors might get noticed earlier this way.

…into OPN_chunking

… if opn messages are longer than one chunk

…o remove asymmetric header decoding from processOPN

…N and into chunking

-Added todo for send chunking. probably needs major restructuring

-Added SecurityPolicy -Added SecurityContexts -Added SecurityPolicy_None -Modified SecureChannel and ServerConfig to accomodate for SecurityPolicies

…olicies

…steps example

…olicies

CLAassistant · 2017-08-21T18:33:17Z

All committers have signed the CLA.

mlgiraud · 2017-08-21T18:48:57Z

If you like, i can remove the rest of the noise as well. Some files just have some includes changed. Those are mainly because the file was used, but not included directly.

…ings

Pro · 2017-08-22T08:27:35Z

examples/server.c

+    UA_ByteString cert = loadCertificate();
+
+    UA_ServerConfig *config = UA_ServerConfig_new_minimal(16664, &cert);
+    if(config == NULL)


The cert variable can already be deleted here, since it is copied in UA_ServerConfig_new_minimal (as it is already in master).
This also avoids memleaks on the return -1 cases

Pro · 2017-08-22T08:29:12Z

plugins/ua_accesscontrol_default.h

@@ -5,6 +5,7 @@
 #define UA_ACCESSCONTROL_DEFAULT_H_

 #include "ua_server.h"
+#include "ua_types_generated.h"


Is this really needed here?
Sometimes CLion adds includes even if the are not needed here...

Pro · 2017-08-22T08:31:13Z

plugins/ua_config_standard.h

@@ -12,6 +12,7 @@ extern "C" {
 #include "ua_server_config.h"
 #include "ua_client.h"
 #include "ua_client_highlevel.h"
+#include "ua_types.h"


Is this needed? Try to move it into .c file where it is needed or the specific header file

Pro · 2017-08-22T08:32:41Z

src/server/ua_securechannel_manager.c

@@ -5,12 +5,14 @@
 #include "ua_securechannel_manager.h"
 #include "ua_session.h"
 #include "ua_server_internal.h"
+#include "ua_transport_generated_handling.h"
+#include "ua_types_generated_handling.h"


Same here, check if the additional includes are really needed.

Pro · 2017-08-22T08:34:12Z

src/server/ua_server_discovery.c

@@ -5,6 +5,8 @@
 #include "ua_server_internal.h"
 #include "ua_client.h"
 #include "ua_config_standard.h"
+#include "ua_types_generated.h"
+#include "ua_types_generated_handling.h"


Same here... Porbably not needed, because the .c file did not change, so it should not require new includes?

Pro · 2017-08-22T08:34:29Z

src/server/ua_services_discovery_multicast.c

@@ -5,6 +5,7 @@
 #include "ua_server_internal.h"
 #include "ua_services.h"
 #include "ua_mdns_internal.h"
+#include "ua_connection_internal.h"


Check include

Pro · 2017-08-22T08:34:36Z

src/server/ua_services_nodemanagement.c

@@ -4,6 +4,8 @@

 #include "ua_server_internal.h"
 #include "ua_services.h"
+#include "ua_types_generated.h"
+#include "ua_types_generated_handling.h"


Check include

These includes are needed afaik but can be omitted, since they are included in the other header files, that are included here. I can remove them.

…d error. This may need some further investigation

Pro · 2017-08-22T08:59:58Z

plugins/ua_config_standard.c

@@ -9,6 +9,7 @@
 #include "ua_securitypolicy_none.h"
 #include "ua_types.h"
 #include "ua_types_generated_handling.h"
+#include "ua_client_highlevel.h"


CLion's fault? The config should not include client stuff.

No. This struct is only defined in the client file and is used in line 305 in the default config, since it supplies config for both server and client.

@jpfr Should we put the UA_SubscriptionSettings struct into another more fitting header? E.g. ua_server_config.h to avoid including client functionality here.

Keep in mind, that the client config struct is needed as well. This is included through that header as well (although transitively)

Hmm, right... 👍
Since the include is now in the source file it shouldn't have that much impact on compilation and Co. So IMHO we can leave it like this for now

Though it might be worth a thought to split the default client and default server config into two separate files.

👍 I would prefer that

Separate PR or this one? I'd really prefer if this one is merged soon, since the conflicts are getting out of hand.

I'd create a new PR based on master for the separate configs and also integrate it in this PR.

IMHO this PR can then be merged, but let's wait for @jpfr

Yeah, you're right. I'd forgotten that the config stuff is on master already.

…olicies

coveralls · 2017-08-24T09:27:42Z

Coverage decreased (-1.9%) to 82.711% when pulling 76599b4 on Infinity95:feature_SecurityPolicies into 9db1938 on open62541:master.

Pro · 2017-08-24T09:33:21Z

Finally the CI tests run through again.

@Infinity95 can you think of a few unit test which you could write? If yes, you could create them in a new PR

mlgiraud · 2017-08-24T09:37:15Z

Sure. Although I'd first like to change the certificate handling, since that is still somewhat ugly and non useful. I think i already mentioned this, but the certificates would get their own CertificateManager plugin, where certificates are stored and can be validated.
This would require some minor changes to the plugin api.
EDIT: Of course in a separate pull request. The merges are getting out of hand.

mlgiraud · 2017-08-28T16:56:29Z

Is there any further input needed from me before this can be merged?

Pro · 2017-08-28T22:45:00Z

@Infinity95 if you have time, you could create the PR for the separate configurations to master. Because I think that we first need to merge that, and than we can finally merge this veeeeery nice PR

mlgiraud · 2017-08-28T23:57:55Z

You mean splitting the config into client and server part?

Pro · 2017-08-29T06:04:41Z

Yes, that shouldn't be that much of a change but makes the code more understandable.

jpfr · 2017-08-29T06:56:13Z

@Infinity95 Can you hold your horses for a little bit?
I'm squashing the many commits into fewer feature-commits.

mlgiraud · 2017-08-29T15:31:33Z

I finally found the bug i mentioned in this comment. Should i wait pushing the fix to this branch? (It's already pushed to my other branch: mlgiraud@32e280c)
This should be included before merging.

jpfr · 2017-10-08T10:33:34Z

Merged via #1187

mlgiraud added 30 commits February 9, 2017 12:03

Fixed mismatching function declaration and the resulting build error

b74badc

Started work on making chunking possible with OPN messages

12c4dae

Fix magic number

1ca5d65

Merge remote-tracking branch 'upstream/master' into OPN_chunking

86dd897

Merge branch 'OPN_chunking' of https://github.com/Infinity95/open62541 …

d554990

…into OPN_chunking

Changed securechannelmanager to allow temporary channels for chunking…

3ef6445

… if opn messages are longer than one chunk

Fix variable naming

9507802

Started refactoring processChunks

49b77e4

More refactoring in processChunks

62f0826

Merge remote-tracking branch 'upstream/master' into OPN_chunking

52aabb6

-More work on refactoring asymmetric message chunking. Next step is t…

3ae02ed

…o remove asymmetric header decoding from processOPN

-Moved sequence header and asymmetricsecurity header out of processOP…

79f6c4a

…N and into chunking

-Variable renaming

9727615

-Added todo for send chunking. probably needs major restructuring

Started introducing SecurityPolicy stuff.

415ce64

-Added SecurityPolicy -Added SecurityContexts -Added SecurityPolicy_None -Modified SecureChannel and ServerConfig to accomodate for SecurityPolicies

Started adding security features to asymmetric chunk recieving.

a51003c

Added securitypolicy support to endpoints creation

ea88915

Merge remote-tracking branch 'upstream/master' into feature_SecurityP…

4c817c5

…olicies

Fixed some minor bugs and added security policy stuff to server_first…

531ab6b

…steps example

Fix compiler warning

6724de9

Fix compiler warning and server_readspeed example

5cf9d28

Added includes for more clarity

2a7e485

Refactored methods for more clarity

81c54a4

Merge master

172b700

Code style

3b0e260

Fix compiler warnings

d0215ab

Add securitypolicies to server example

c99f526

Remove memory leak in server_new

f16008b

Added decryption and verification to symmetric chunks

a717482

Added key generation calls

caaac2b

Fixed warning

e0beb67

Merge remote-tracking branch 'upstream/master' into feature_SecurityP…

3582fd2

…olicies

Merge branch 'master' into feature_SecurityPolicies

035991a

mlgiraud added 2 commits August 21, 2017 23:57

Remove windows include

6fab680

Add securitypolicy_none to test plugin sources and normalize line end…

44c655d

…ings

Pro reviewed Aug 22, 2017

View reviewed changes

mlgiraud added 2 commits August 22, 2017 10:46

Fix incorrect delete parameter and make valgrind ignore noai6ai_cache…

f032ea8

…d error. This may need some further investigation

Remove includes

56c8d25

Pro reviewed Aug 22, 2017

View reviewed changes

mlgiraud and others added 6 commits August 22, 2017 11:10

Fix memory leak in server example

c0abca8

Fix build errors in unit tests

48611c8

Merge remote-tracking branch 'upstream/master' into feature_SecurityP…

efb08b9

…olicies

Reenable fail on error

cf4cd51

Disable pthread cache on valgrind

57b81e6

Merge remote-tracking branch 'upstream/master' into feature_SecurityP…

76599b4

…olicies

Pro mentioned this pull request Sep 14, 2017

0.3 release - open issues #1205

Closed

5 tasks

jpfr closed this Oct 8, 2017

jpfr deleted the feature_SecurityPolicies branch December 15, 2018 11:05

lock bot locked as resolved and limited conversation to collaborators Dec 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Security Policies as Plugins #1003

Feature: Security Policies as Plugins #1003

mlgiraud commented Mar 31, 2017 •

edited

CLAassistant commented Aug 21, 2017 •

edited

mlgiraud commented Aug 21, 2017

Pro Aug 22, 2017

Pro Aug 22, 2017

Pro Aug 22, 2017

Pro Aug 22, 2017

Pro Aug 22, 2017

Pro Aug 22, 2017

Pro Aug 22, 2017

mlgiraud Aug 22, 2017

Pro Aug 22, 2017

mlgiraud Aug 22, 2017

Pro Aug 22, 2017

mlgiraud Aug 22, 2017

Pro Aug 22, 2017

mlgiraud Aug 24, 2017

Pro Aug 24, 2017

mlgiraud Aug 24, 2017

Pro Aug 24, 2017

mlgiraud Aug 24, 2017

coveralls commented Aug 24, 2017

Pro commented Aug 24, 2017

mlgiraud commented Aug 24, 2017 •

edited

mlgiraud commented Aug 28, 2017

Pro commented Aug 28, 2017

mlgiraud commented Aug 28, 2017

Pro commented Aug 29, 2017

jpfr commented Aug 29, 2017

mlgiraud commented Aug 29, 2017

jpfr commented Oct 8, 2017

Feature: Security Policies as Plugins #1003

Feature: Security Policies as Plugins #1003

Conversation

mlgiraud commented Mar 31, 2017 • edited

CLAassistant commented Aug 21, 2017 • edited

mlgiraud commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 24, 2017

Pro commented Aug 24, 2017

mlgiraud commented Aug 24, 2017 • edited

mlgiraud commented Aug 28, 2017

Pro commented Aug 28, 2017

mlgiraud commented Aug 28, 2017

Pro commented Aug 29, 2017

jpfr commented Aug 29, 2017

mlgiraud commented Aug 29, 2017

jpfr commented Oct 8, 2017

mlgiraud commented Mar 31, 2017 •

edited

CLAassistant commented Aug 21, 2017 •

edited

mlgiraud commented Aug 24, 2017 •

edited