Open phonexicum opened 3 months ago
I deliberately provided CertServer
protected property instead of forcing implementers to instantiate it every time for several reasons:
CertServerModule.CreateExit
in policy module and CertServerModule.CreatePolicy
in exit module.CertServerModule
creates some COM objects and method definitions extractions into actions and funcs (https://github.com/PKISolutions/ADCS-CertMod/blob/master/ADCS.CertMod.Managed/CertServerExitPolicyManaged.cs) which will negatively affect performance and add heavy memory management (heap/COM allocations/releases) under heavy load. In addition, CertServer
property will have to implement IDisposable
and cause memory leaks if object created inside VerifyRequest
is not properly disposed. For simplicity purposes, I created this object as singleton with same lifetime as CA process.I did test this module under some load, but didn't encounter race conditions. Maybe my tests weren't sufficient. But anyways, even in current implementation it is relatively easy to address race condition issue by using locks:
public class MyPolicyModule : CertPolicyBase {
readonly Object _myLock = new();
// the rest is omitted for brevity
public override PolicyModuleAction VerifyRequest(String strConfig, Int32 Context, Int32 bNewRequest, Int32 Flags) {
lock (_myLock) {
// implement your logic
// it may be worth to copy parameters into local variables as soon as possible.
}
}
// the rest is omitted for brevity
}
Of course, this will slightly slow things down, because multiple threads will stay in queue while awaiting lock release and execute one-by-one (synchronously). I think that this approach should reduce race condition chances. What do you think?
But, according to your proposal, as a developer, I am now placed between two sacrificial choices:
For sure, ignoring errors is not my choice. But do I have an opportunity to develop race-condition -free multi-threaded policy plugin? - It looks like "no", because:
CertPolicyBase::VerifyRequest
as its first action performs CertServer.InitializeContext(Context);
at CertPolicyBase.cs
base.VerifyRequest
call using locks.
Thus, we hit the "chicken and egg" problem:
CertServer
interface for a developer to increase the module's performance under heavy load (which will negatively affect performance and add heavy memory management (heap/COM allocations/releases) under heavy load
)base.VerifyRequest
call and around some part of the currently developed policy module plugin;From the considerations described previously, I see two consequences:
The interface CertServer
is currently prone to race-condition issue.
However, the interface is useful according to the reasons you have described.
To me the conclusion is simple: the CertServer
should become safe. Perhaps, it can be achieved by some thread synchronization technique, for example, locking the object in CertServerModule::InitializeContext
and releasing it in CertServerModule::FinalizeContext
.
Or maybe you will find some other thread synchronization solution more appropriate here.
Just like I explained, currently I must execute base.VerifyRequest
under lock to make myself thread-safe, although I don't use CertServer
in my code and the original Microsoft's native module doesn't use CertServer
object either.
I could write the safe multi-threaded code if I only could execute funcVerifyRequest.Invoke(strConfig, Context, bNewRequest, Flags);
(from here) without messing up with CertServer
(from here). However the property funcVerifyRequest
is "private" and not available in a derived class.
I also understand that backward compatibility is very important for projects like this, therefore can some new interface (similar to base.VerifyRequest
) be provided to make it possible to completely avoid CertServer
initialization and usage to write multi-threaded and safe code?
Look, Microsoft CA's throughput is much higher than your policy module's throughput, because CA is COM server (native code), your policy module is .NET runtime (just wrapped in CCW). .NET runtime is always slower than native code, this is not debatable. Not to mention that there is additional overhead in marshalling data between COM and .NET runtime (in both directions), which slows things even more. So the answer to your question:
But do I have an opportunity to develop race-condition -free multi-threaded policy plugin?
is "no", unfortunately and it is not because how I designed this framework.
Under high load, .NET runtime is a bottleneck and you may run out of memory if high load is sustained for reasonable period, because CA will make calls faster than your ability to process. This is a classic problem. If you are looking for a policy module which performance would be comparable to CA performance, you would write it using native code (C++). Then you can have fully multi-threaded and thread safe policy/exit module. .NET is a RAD platform that allows you to implement your arbitrary logic quickly. However this convenience comes at cost: performance. I put all reasonable efforts to make the code as performant as possible, by keeping the balance with maintainability and easiness/usefulness. When I hit such performance problem, I prefer to slow things down rather than allowing uncontrolled parallel thread creation and crashing entire server when it runs out of memory.
- Or maybe you will find some other thread synchronization solution more appropriate here.
thread synchronization turns your code (in critical sections) into single-threaded code by definition. It doesn't matter much how exactly you implement this synchronization. Though, it may be possible to make some kind of controlled thread pool. Say, have 100 threads (with singleton CertServer instance in each) in the pool and allow to run up to 100 threads in parallel (each thread will have its own context) and block new thread creation when pool is exhausted until some threads are released. This should boost throughput to some extent. The question then, what is the "optimal" number of parallel threads and how to test this. As you may already noticed, it is not easy to make policy module testing (both, logic and performance).
2. Just like I explained, currently I must execute
base.VerifyRequest
under lock to make myself thread-safe, although I don't useCertServer
in my code and the original Microsoft's native module doesn't useCertServer
object either. I could write the safe multi-threaded code if I only could executefuncVerifyRequest.Invoke(strConfig, Context, bNewRequest, Flags);
(from here) without messing up withCertServer
(from here). However the propertyfuncVerifyRequest
is "private" and not available in a derived class.
You are not required to use protected CertServer
object. Inside your VerifyRequest
override, you can create your own instance of CertServerModule
by calling CertServerModule.CreatePolicy
or use anything else at your choice. You take all the responsibility to ensure thread safety in your VerifyRequest
override then.
You are not required to use protected
CertServer
object. Inside yourVerifyRequest
override, you can create your own instance ofCertServerModule
by callingCertServerModule.CreatePolicy
or use anything else at your choice. You take all the responsibility to ensure thread safety in yourVerifyRequest
override then.
That is the first thing I have done, after I hit errors on ADCS due to the problem of race-condition.
But it is not enough to just "not use CertServer
".
If I want to call the native microsoft's policy module, I am forced to write the following code to make my implementation of VerifyRequest
thread-safe:
public class MyPolicyModule : CertPolicyBase {
readonly Object _myLock = new();
// the rest is omitted for brevity
public override PolicyModuleAction VerifyRequest(String strConfig, Int32 Context, Int32 bNewRequest, Int32 Flags) {
lock (_myLock) {
PolicyModuleAction nativeResult = base.VerifyRequest(strConfig, Context, bNewRequest, Flags);
// base.VerifyRequest contains: "CertServer.InitializeContext(Context);" which is NOT thread-safe
CertServer.FinalizeContext();
}
// ...
// my logic goes here, and it is thread-safe
// ...
}
// the rest is omitted for brevity
}
Currently I see no way to call microsoft's native policy module using ADCS-CertMod framework without using locks. And it results in a partially single-threaded policy module.
Currently I see no way to call microsoft's native policy module using ADCS-CertMod framework without using locks.
As described above, it is not recommended to have individual and separate instance of CertServerModule
because of performance and memory overhead under heavy load which may lead to server crash, which is not a desirable outcome. The only viable solution I see is either, make VerifyRequest
completely single-threaded, or introduce a pool of CertServerModule
objects which will make the method thread-safe to certain extent and then turn VerifyRequest
single-threaded if pool is exhausted. For example, Microsoft CLM policy module uses a pool of CertServerPolicy
objects:
and then in VerifyRequest
they do:
and this is what I'm suggesting. Though it isn't something possible without introducing breaking changes. The idea is that VerifyRequest
will no longer be virtual and separate abstract method will be introduced. Let's say this:
// not virtual anymore
public PolicyModuleAction VerifyRequest(String strConfig, Int32 Context, Int32 bNewRequest, Int32 Flags) {
CertServerModule certServer = pool.GetNext();
CertServer.InitializeContext(Context);
PolicyModuleAction nativeResult = funcVerifyRequest.Invoke(strConfig, Context, bNewRequest, Flags);
try {
return VerifyRequest(certServer, nativeResult);
} finally {
certServer.FinalizeContext();
pool.Release(certServer);
}
}
// can duplicate original parameters if it makes sense
protected abstract PolicyModuleAction VerifyRequest(CertServerModule certServer, PolicyModuleAction nativeResult);
And then your implementation of abstract VerifyRequest
will be thread-safe by default and resources are released automatically, no need to do it yourself.
Directly calling funcVerifyRequest.Invoke(strConfig, Context, bNewRequest, Flags);
indeed will be a sufficient workaround for me to completely avoid usage of CertServer
.
However, the funcVerifyRequest
property is private (not protected), and therefore not available to me.
I have no plans to expose this func to inheritors, this is to ensure that this method is not misused. In addition, I want to ensure that this func is always called before running your own code.
Can a new interface be introduced into the CertPolicyBase
class?
To provide a developer the ability to call funcVerifyRequest.Invoke
in a multi-threaded way without touching thread-unsafe CertServer
property?
For example:
public PolicyModuleAction VerifyRequest2(String strConfig, Int32 Context, Int32 bNewRequest, Int32 Flags) {
return funcVerifyRequest.Invoke(strConfig, Context, bNewRequest, Flags);
}
Thus a developer will be able to call base.VerifyRequest2(strConfig, Context, bNewRequest, Flags)
without the need to make the code partially single-threaded to make it thread-safe:
public class MyPolicyModule : CertPolicyBase {
// readonly Object _myLock = new(); // NOT NEEDED any more
// the rest is omitted for brevity
public override PolicyModuleAction VerifyRequest(String strConfig, Int32 Context, Int32 bNewRequest, Int32 Flags) {
// lock (_myLock) { // NOT NEEDED any more
PolicyModuleAction nativeResult = base.VerifyRequest2(strConfig, Context, bNewRequest, Flags);
// the call is thread-safe, because thread-unsafe `CertServer` is not used
// CertServer.FinalizeContext();
// }
// ...
// my logic goes here, and it is thread-safe
// ...
}
// the rest is omitted for brevity
}
I have made the draft PR for my last proposition.
Hi, first of all, thank you for this project, it is awesome.
problem description
During development of policy module for the ADCS, I run into a problem of race conditions. It turns out the ADCS (certsrv.exe) service under high load may execute
ICertPolicy::VerifyRequest
concurrently in different threads. And if one will use in VerifyRequest some shared resources (for exampleCertPolicyBase.CertServer
), it may result in unpredictable behavior.Examples of errors could be:
Eventually errors result in a broken ADCS service, and manual service restart was required.
The problem could be simply solved: I just didn't used
CertPolicyBase.CertServer
provided by abstact class and instead created and initialized new instance ofCertServerModule
in everyVerifyRequest
function call, without any sharing between threads.problem interpretation
For a reason described above, current approach (of providing access to the property
CertPolicyBase.CertServer
for a developer) looks error prone.I believe, that the project ADCS-SID-Extension-Policy-Module has the same problems with race conditions, which will only reveal itself under high load, because the
CertServer
is used in a couple of places atPolicy.cs
file.I believe, additionally race conditions may result in:
CertServerModule::FinalizeContext()
(e.g. at policy module)CertServer.InitializeContext(Context);
(e.g. at base.VerifyRequest) in case when two threads initialize CertServer, but only memory allocated by second thread will be freed later. (unfortunately, thisInitializeContext
seems to be unavoidable if the developer wish to use for example microsoft's native policy module)proposition
I propose to completely remove
CertServer
property from abstract classCertPolicyBase
. If the developer requires the object to read or edit properties of a certificate, he must instantiate new object in each VerifyRequest thread.