Alex, Denis, Seems like security API is indeed a bit over-engineered.
Let's get rid of SecurityContext and use SecuritySubject instead. > SecurityContext is just a POJO wrapper over > SecuritySubject's > org.apache.ignite.plugin.security.SecuritySubject#permissions. > It's functionality can be easily moved to SecuritySubject. I totally agree. Both subject and context are implemented by plugin provider, and I don't see any reason to keep both abstractions, especially if we are going to get rid of transferring subject in node attributes (argument that subject is more lightweight won't work anymore). Also, there's kind of mess in node authentication logic. There are at least two components responsible for it: DiscoverySpiNodeAuthenticator (which is forcibly set by GridDiscoveryManager, but in fact public) and GridSecurityProcessor (which performs actual node auth logic, but private). I also don't understand why we need both #authenticate(AuthenticationContext) and #authenticateNode(ClusterNode, SecurityCredentials) methods while it's possible to set explicit SecuritySubjectType.REMOTE_NODE in AuthenticationContext (this is arguable; perhaps there are strong reasons). Finally, areas of responsibility between IgniteSecurity and GridSecurityProcessor are kind of mixed. As far as I understand, the first is responsible for Ignite-internal management of security logic (keeping thread-local context, caching security contexts, etc; we don't expect IgniteSecurity to be replaced by plugin provider) and the latter is responsible for user-custom authentication / authorization logic. To be honest, it took plenty of time to figure this out for me. >From my point of view, we should make GridSecurityProcessor interface public, rename it (it requires plenty of time to find the difference from IgniteSecurity), make its API as simple and non-duplicating as possible and clarify its area of responsibility (e.g. should it be responsible for propagation of successfully authenticated subject among all nodes or not?) to make it easy to embed custom security logic in Ignite. Regarding thin clients fix: implementation made by Denis suits better to the very implicit contract that it's better to change API contracts of an internal IgniteSecurity than of internal GridSecurityProcessor (which actually mustn't be internal). > My approach doesn't require any IEPs, just minor change in code and to > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > contract. Looks like a misuse of #authenticate method to me. It should perform initial authentication based on credentials (this may include queries to external authentication subsystem, e.g. LDAP). User may want to don't authenticate thin client on every node (this will increase the number of requests to auth subsystem unless user implicitly implements propagation of thin clients' contexts between nodes and make #authenticate cluster-wide idempotent: first call should perform actual authentication, next calls should retrieve context of already authenticated client). Presence of the separate #securityContext(UUID) highlights that user indeed should care about propagation of thin clients' contexts between the cluster nodes. -- Ivan On Fri, Mar 20, 2020 at 12:22 PM Veena Mithare <v.mith...@cmcmarkets.com> wrote: > Hi Alexei, Denis, > > One of the main usecases of thin client authentication is to be able to > audit the changes done using the thin client user. > To enable that : > We really need to resolve this concern as well : > https://issues.apache.org/jira/browse/IGNITE-12781 > > ( Incorrect security subject id is associated with a cache_put event > when the originator of the event is a thin client. ) > > Regards, > Veena > > > -----Original Message----- > From: Alexei Scherbakov <alexey.scherbak...@gmail.com> > Sent: 18 March 2020 08:11 > To: dev <dev@ignite.apache.org> > Subject: Re: Security Subject of thin client on remote nodes > > Denis Garus, > > Both variants are capable of solving the thin client security context > problem. > > My approach doesn't require any IEPs, just minor change in code and to > > org.apache.ignite.internal.processors.security.IgniteSecurity#authenticate(AuthenticationContext) > contract. > We can add appropriate documentation to emphasize this. > The argument "fragile" is not very convincing for me. > > I think we should collect more opinions before proceeding with IEP. > > Considering a fact we actually *may not care* about compatibility (I've > already explained why), I'm thinking of another approach. > Let's get rid of SecurityContext and use SecuritySubject instead. > SecurityContext is just a POJO wrapper over SecuritySubject's > org.apache.ignite.plugin.security.SecuritySubject#permissions. > It's functionality can be easily moved to SecuritySubject. > > What do you think? > > > > пн, 16 мар. 2020 г. в 15:47, Denis Garus <garus....@gmail.com>: > > > Hello, Alexei! > > > > I agree with you if we may not care about compatibility at all, then > > we can solve the problem much more straightforward way. > > > > In your case, the method GridSecurityProcessor#authenticate will have > > an implicit contract: > > [ if actx.subject() != null then > > returns SecurityContext > > else > > do authenticate ] > > > > It looks fragile. > > > > When we extend the GridSecurityProcessor, there isn't this problem: > > we have the explicit contract and can make default implementation that > > throws an unsupported operation exception to enforcing compatibility > > check. > > > > In any case, we need to change GridSecurityProcessor implementation. > > > > But I think your proposal to try to find a security context in the > > node's attributes first is right for backward compatibility when > > Ignite users don't use thin clients. > > > > Summary: > > I suggest adding a new method to GridSecurityProcessor because it has > > a clear contract and enforces compatibility check natural way. > > > > вс, 15 мар. 2020 г. в 17:13, Alexei Scherbakov < > > alexey.scherbak...@gmail.com > > >: > > > > > Denis Garus, > > > > > > I've looked at the IEP proposed by you and currently I'm thinking > > > it's > > not > > > immediately required. > > > > > > The problem of missing SecurityContexts of thin clients can be > > > solved > > much > > > easily. > > > > > > Below is the stub of a fix, it requires correct implementation of > > > method > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > #authenticatedSubject > > > by GridSecurityProcessor: > > > > > > /** {@inheritDoc} */ > > > @Override public OperationSecurityContext withContext(UUID nodeId) > { > > > try { > > > SecurityContext ctx0 = secCtxs.get(nodeId); > > > > > > if (ctx0 == null) { > > > ClusterNode node = > > > Optional.ofNullable(ctx.discovery().node(nodeId)) > > > .orElseGet(() -> > > > ctx.discovery().historicalNode(nodeId)); > > > > > > // This is a cluster node. > > > if (node != null) > > > ctx0 = nodeSecurityContext(marsh, > > > U.resolveClassLoader(ctx.config()), findNode(nodeId)); > > > else { > > > // This is already authenticated thin client. > > > SecuritySubject subj = > > > authenticatedSubject(nodeId); > > > > > > assert subj != null : "Subject is null " + > > > nodeId; > > > > > > AuthenticationContext actx = new > > > AuthenticationContext(); > > > actx.subject(subj); > > > > > > ctx0 = secPrc.authenticate(actx); > > > } > > > } > > > > > > secCtxs.putIfAbsent(nodeId, ctx0); > > > > > > return withContext(ctx0); > > > } catch (IgniteCheckedException e) { > > > throw new IgniteException(e); > > > } > > > > > > The idea is to create a thin client SecurityContext on a node not > > > having > > a > > > local context using existing SecuritySubject data. > > > > > > Method > > > > > org.apache.ignite.internal.processors.security.GridSecurityProcessor#a > > uthenticate > > > should check for not null SecuritySubject field and just recreate > > > SecurityContext using passed info (because it's already authenticated). > > > > > > We have all necessary information in SecuritySubject returned by > > > > > > > > org.apache.ignite.internal.processors.security.IgniteSecurityProcessor > > #authenticatedSubject > > > by GridSecurityProcessor method. > > > > > > Because it is internal API, we may not care about compatibility at > > > all, but nevertheless it is possible to add compatibility check in > > > the method above. If a feature is not supported the operations from > > > thin clients should be forbidden. > > > > > > You proposal has the similar problem: if GridSecurityProcessor does > > > not support retriving context for thin clients, such clients will > > > not be able to proceed with operation. > > > > > > Still, the cleanup of security API is necessary and should be done > > > in 3.0 > > > > > > > > > > > > чт, 12 мар. 2020 г. в 16:48, VeenaMithare <v.mith...@cmcmarkets.com>: > > > > > > > HI , > > > > > > > > Created this jira : > > > > https://issues.apache.org/jira/browse/IGNITE-12781 > > > > > > > > regards, > > > > Veena. > > > > > > > > > > > > > > > > -- > > > > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ > > > > > > > > > > > > > -- > > > > > > Best regards, > > > Alexei Scherbakov > > > > > > > > -- > > Best regards, > Alexei Scherbakov > ________________________________ > > > Spread bets and CFDs are complex instruments and come with a high risk of > losing money rapidly due to leverage. 70.5% of retail investor accounts > lose money when spread betting and/or trading CFDs with this provider. You > should consider whether you understand how spread bets and CFDs work and > whether you can afford to take the high risk of losing your money. > > Professional clients: Losses can exceed deposits when spread betting and > trading CFDs. Countdowns carry a level of risk to your capital as you could > lose all of your investment. These products may not be suitable for all > clients therefore ensure you understand the risks and seek independent > advice. Invest only what you can afford to lose. > > CMC Markets UK plc (173730) and CMC Spreadbet plc (170627) are authorised > and regulated by the Financial Conduct Authority in the United Kingdom. CMC > Markets UK plc and CMC Spreadbet plc are registered in England and Wales > with Company Numbers 02448409 and 02589529 and with their registered > offices at 133 Houndsditch, London, EC3A 7BX. > > The content of this e-mail (including any attachments) is strictly > confidential and is for the sole use of the intended addressee(s). If you > are not the intended recipient of this e-mail please notify the sender > immediately and delete this e-mail from your system. Any disclosure, > copying, dissemination or use of its content (including any attachments) is > strictly prohibited. CMC Markets UK plc and CMC Spreadbet plc reserve the > right to intercept and monitor the content of the e-mail messages to and > from its systems. > > E-mails may be interfered with or may contain viruses or other defects for > which CMC Markets UK plc and CMC Spreadbet plc accept no responsibility. It > is the responsibility of the recipient to carry out a virus check on the > e-mail and any attachment(s). > > This communication is not intended as an offer or solicitation for the > purchase or sale of a financial instrument or as an official confirmation > of any transaction unless specifically presented as such. > > CMC Markets UK plc and CMC Spreadbet plc are registered in England and > Wales with Company Numbers 02448409 and 02589529 and with their registered > offices at 133 Houndsditch, London, EC3A 7BX. >
