Port System.Security.Cryptography.RijndaelManaged, limited to AES #9984

Closed
danmosemsft opened this Issue Jul 11, 2016 · 10 comments

Projects

None yet

4 participants

@danmosemsft
Member

This type is getting significant use.

T:System.Security.Cryptography.RijndaelManaged

We should bring it back as functional, but to make it just wrap Aes.Create(), and fail for any Rijndael settings which are not AES settings (since it looks like our Rijndael implementation restricted it to 128/192/256-bit keys already that really just means use a fixed block size). And make it be [EditorBrowsable(Never)] so IntelliSense never suggests it to someone (and we’ll hide it in a different contract/implementation assembly so it only gets picked up by the mscorlib forwarder anyways).

Existing code may be copying extraordinarily stale samples which are merely recommending Rijndael-as-AES over DES/3DES.

But, since no one used the Rijndael type directly, and it defines no new members, the compat shim RijndaelManaged should directly extend SymmetricAlgorithm.

So pretty much every member just defers to the Aes.Create() object, with the possible exception of BlockSize:

public int BlockSize
{
    get
    {
        return _aes.BlockSize;
    }
    set
    {
        // Values which were legal in .NET Framework RijndaelManaged but not .NET Core’s shim type
        if (value == 192 || value == 256)
            throw new PlatformNotSupportedException(some resource string saying raw Rijndael is not supported);

        // Any other invalid block size will get the normal “invalid block size” exception.
        _aes.BlockSize = value;
    }
}

(Or, don’t special case it, and let it just throw the invalid block size exception; but that might be bad form)

@danmosemsft danmosemsft added this to the 1.1.0 milestone Jul 11, 2016
@steveharter
Member

Per discussion, these should go into the System.Security.Cryptography.Algorithms package but need to be reviewed by FXDC first

@danmosemsft
Member

@bartonjs should this include M:System.Security.Cryptography.Rijndael.Create ? It is used by 5% of the tier 1.

@bartonjs
Member

@danmosemsft Either I missed that in the report, or there's new data. My previous conclusion was that the Rijndael (base) class had no members that were called, and so should be left out altogether. Instead, I guess the base class should exist (Browsable(Never)) and define Create. Following the convention of the rest of the algorithms it should return an opaque implementation.

@steveharter
Member
steveharter commented Oct 3, 2016 edited

Full set of netstandard 2.0 apis below

public abstract class Rijndael : SymmetricAlgorithm
M:System.Security.Cryptography.Rijndael.#ctor
M:System.Security.Cryptography.Rijndael.Create
M:System.Security.Cryptography.Rijndael.Create(System.String)

public sealed class RijndaelManaged : Rijndael
M:System.Security.Cryptography.RijndaelManaged.#ctor

public sealed class RijndaelManagedTransform : ICryptoTransform, IDisposable
P:System.Security.Cryptography.RijndaelManagedTransform.BlockSizeValue
P:System.Security.Cryptography.RijndaelManagedTransform.CanReuseTransform
P:System.Security.Cryptography.RijndaelManagedTransform.CanTransformMultipleBlocks
P:System.Security.Cryptography.RijndaelManagedTransform.InputBlockSize
P:System.Security.Cryptography.RijndaelManagedTransform.OutputBlockSize
M:System.Security.Cryptography.RijndaelManagedTransform.Clear
M:System.Security.Cryptography.RijndaelManagedTransform.Dispose
M:System.Security.Cryptography.RijndaelManagedTransform.Reset
M:System.Security.Cryptography.RijndaelManagedTransform.TransformBlock(System.Byte[],System.Int32,System.Int32,System.Byte[],System.Int32)
M:System.Security.Cryptography.RijndaelManagedTransform.TransformFinalBlock(System.Byte[],System.Int32,System.Int32)
@bartonjs
Member
bartonjs commented Oct 3, 2016

@weshaggard For types like RijndaelManagedTransform which have no public constructor, do we need to define private RijndaelManagedTransform() in the contract to maintain that representation? (Does our tooling already do this for us?)

@weshaggard
Member

@weshaggard For types like RijndaelManagedTransform which have no public constructor, do we need to define private RijndaelManagedTransform() in the contract to maintain that representation? (Does our tooling already do this for us?)

Yes you will need to explicitly put a private or internal constructor there or the Compiler will make a default public one. GenAPI does do that for you.

@weshaggard weshaggard closed this Oct 3, 2016
@bartonjs
Member
bartonjs commented Oct 3, 2016

(I think Wes just hit the wrong button...)

@bartonjs bartonjs reopened this Oct 3, 2016
@weshaggard
Member

LOL. Yes (That dam close and comment button is just too easy to click).

@steveharter
Member

@danmosemsft can you elaborate on this (and if I need to do anything other than the EditorBrowsable). Thanks

and we’ll hide it in a different contract/implementation assembly so it only gets picked up by the mscorlib forwarder anyways)

@bartonjs
Member

@steveharter I'm guessing that the comment came from me in an email... but I don't remember what, if anything, I would have meant. (Perhaps "define it in the impl but leave it out of the ref"?).

Also, where I said RijndaelManaged should go directly to SymmetricAlgorithm, that's no longer valid, since the Rijndael base class is also part of netstandard2.0 (I was trying to cut that type).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment