Design suggestions for extending DcmStorageSCU with TLS support?

All other questions regarding DCMTK

Moderator: Moderator Team

Post Reply
Message
Author
jogerh
Posts: 20
Joined: Mon, 2022-02-28, 08:55

Design suggestions for extending DcmStorageSCU with TLS support?

#1 Post by jogerh » Thu, 2022-05-12, 20:22

The DcmStorageSCU does not support TLS, and there is no equivalent in dcmtls as far as I can see. I therefore want to add support for TLS in DcmStorageSCU. Do you have any plans for adding TLS support for this base class, or do you have any design ideas? The problem as I see it is that the DCMTK design for TLS is an 'add-on', which requires reimplementation for all classes in the inheritance hierarchy of DcmSCU. It would have been nice if TLS support was composable, such that it could be mixed into DcmSCU as well as DcmStorageSCU. The alternative would be to make TLS a first class citizen of DcmSCU and enabled by preprocessor defines.

Any insight to DCMTK maintainers thoughts on this is highly appreciated.

Thanks,
Jøger
Last edited by jogerh on Fri, 2022-05-13, 09:30, edited 1 time in total.

jogerh
Posts: 20
Joined: Mon, 2022-02-28, 08:55

Re: Design suggestions for extending DcmStorageSCU with TLS support?

#2 Post by jogerh » Fri, 2022-05-13, 09:13

Something like the example below could work. The example is based upon DcmTLSScu, but instead of inheriting from DcmSCU, the base class is templatized. Now, this mixin can be used with both DcmTLS and DcmStorageSCU.

Does anyone see any issues with this design proposal?

Code: Select all


template <typename Base>
class DcmEnableTLS : public Base
{
public:

    /** Constructor, just initializes internal class members
     */
    DcmEnableTLS();

    /** Initialize SCU with connection peer information
     *  @param peerHost    The hostname or IP of the host to talk to
     *  @param peerAETitle AETitle of peer host to be used
     *  @param portNum     TCP/IP port number to talk to
     */
    DcmEnableTLS(const OFString& peerHost,
        const OFString& peerAETitle,
        const Uint16 portNum);

    /** Virtual destructor
      */
    virtual ~DcmEnableTLS();

    /** Initialize network, i.e.\ prepare for association negotiation
     *  @return EC_Normal if initialization was successful, otherwise error code
     */
    virtual OFCondition initNetwork();

    /** Negotiates association by using presentation contexts and parameters
     *  as defined by earlier function calls
     *  @return EC_Normal if negotiation was successful, otherwise error code.
     */
    virtual OFCondition negotiateAssociation();

    /** Closes the association of this SCU
     *  @deprecated The use of this method is deprecated. Please use
     *    DcmSCU::releaseAssociation() or DcmSCU::abortAssociation() instead.
     *  @param closeType Define whether to release or abort the association
     */
    virtual void closeAssociation(const DcmCloseAssociationType closeType);

    /** Add file with trusted certificate (used if authentication is enabled)
     *  @param str Filename
     */
    virtual void addTrustedCertFile(const OFString& str);

    /** Add directory to list of directories containing trusted certificates (used
     *  if authentication is enabled)
     *  @param certDir Not documented yet
     */
    virtual void addTrustedCertDir(const OFString& certDir);

    /** Turn on authentication for TLS
     *  @param privateKey    File with private key to authenticate with
     *  @param certFile      File with certificate to authenticate with
     *  @param passphrase    Password to access key. NULL asks user on console.
     *         "" will send empty password. Default is asking the
     *         user to enter password.
     *  @param privKeyFormat Format of private key parameter. Default is
     *                       SSL_FILETYPE_PEM.
     *  @param certFormat    Format of certificate file parameter. Default is
     *                       SSL_FILETYPE_PEM.
     */
    virtual void enableAuthentication(const OFString& privateKey,
        const OFString& certFile,
        const char* passphrase = NULL,
        const DcmKeyFileFormat privKeyFormat = DCF_Filetype_PEM,
        const DcmKeyFileFormat certFormat = DCF_Filetype_PEM);

    /** Disables authentication. However, DcmEnableTLS will try to establish secured
     *  connection in terms of encrypting data. Default is that authentication is disabled.
     */
    virtual void disableAuthentication();

    /** replace the current list of ciphersuites by the list of ciphersuites
     *  for the given profile. Caller must ensure that initNetwork() is executed before this call.
     *  @param profile TLS Security Profile
     *  @return EC_Normal if successful, an error code otherwise
     */
    virtual OFCondition setTLSProfile(DcmTLSSecurityProfile profile);

    /** adds a ciphersuite to the list of ciphersuites for TLS negotiation.
     *  Caller must ensure that initNetwork() is executed before this call.
     *  It is the responsibility of the user to ensure that the added ciphersuite
     *  does not break the rules of the selected profile. Use with care!
     *  @param suite TLS ciphersuite name, in the official TLS name form.
     *  @return EC_Normal if successful, an error code otherwise
     */
    virtual OFCondition addCipherSuite(const OFString& suite);

    /** Set file to be used as random seed for initializing the Pseudo Random
     *  Number Generator (PRNG)
     *  @param seedFile The seed file to be used.
     */
    virtual void setReadSeedFile(const OFString& seedFile);

    /** Set file to be use to store the updated random seed to store the updated
     *  random seed from the Pseudo Random Number Generator (PRNG).
     *  @param seedFile The seed file to be used for writing back seed info
     */
    virtual void setWriteSeedFile(const OFString& seedFile);

    /** Set whether peer's certificate must be there, only is checked if there or is ignored
     *  @param cert Peer certificate verification mode. The following values are permitted:
     *         DCV_requireCertificate, DCV_checkCertificate, DCV_ignoreCertificate
     */
    virtual void setPeerCertVerification(const DcmCertificateVerification cert);

    /** Set Diffie-Hellman parameters from file. This method should be
     *  called after calls to initNetwork() and setTLSProfile().
     *  @param filename of dhParam Diffie-Hellman parameter file to be used.
     */
    virtual void setDHParam(const OFString& dhParam);

    /** Returns OFTrue if authentication is enabled
     *  @param privKeyFile   The file containing the private key used
     *  @param certFile      The file containing the certificate used
     *  @param passphrase    The passphrase used for unlocking the private key
     *         file. If NULL, password is asked from STDIN. If empty string (""),
     *         password is sent empty. All others values are sent as given.
     *  @param privKeyFormat Format of the private key in privKeyFile, see
     *         documentation of m_privateKeyFileFormat.
     *  @param certFormat    The certificate format of certificate in certFile.
     *         See documentation of m_certKeyFileFormat.
     *  @return Returns OFTrue if authentication is enabled, OFFalse otherwise
     */
    virtual OFBool getAuthenticationParams(OFString& privKeyFile,
        OFString& certFile,
        const char*& passphrase,
        int& privKeyFormat,
        int& certFormat) const;

    /** Get files considered for trusted certificates.
     *  @param files The file names that are considered as trusted
     *         certificates
     */
    virtual void getTrustedCertFiles(OFList<OFString>& files /*out*/) const;

    /** Get directories containing considered to contain trusted certificates.
     *  @param trustedDirs directories considered to contain trusted certificates.
     */
    virtual void getTrustedCertDirs(OFList<OFString>& trustedDirs /*out*/) const;

    /** Get random seed file used for initializing Pseudo Random Number
     *  Generator (PRNG)
     *  @return Random seed file used for reading
     */
    virtual OFString getReadSeedFile() const;

    /** Get random seed file the PRNG should use to store back updated random
     *  seed information
     *  @return Get random seed file used for writing back updated seed
     */
    virtual OFString getWriteSeedFile() const;

protected:
    /** Factory function for new TLS transport layer instances.
     *  This function is used by initNetwork to create the transport layer.
     *  Override this function to allow custom configuration of TLS, and allow
     *  configuring the transport layer with certificates from third party
     *  certificate stores.
     *  @return a new TLS transport layer instance or NULL in case of failure
     */
    virtual DcmTLSTransportLayer* CreateTransportLayer();
private:

    /** Private undefined copy-constructor. Shall never be called.
     *  @param src Source object
     */
    DcmEnableTLS(const DcmEnableTLS& src);

    /** Private undefined operator=. Shall never be called.
     *  @param src Source object
     *  @return Reference to this object
     */
    DcmEnableTLS& operator=(const DcmEnableTLS& src);

    /// The TLS layer responsible for all encryption/authentication stuff
    DcmTLSTransportLayer* m_tLayer;

    /// If enabled, authentication of client/server is enabled
    OFBool m_doAuthenticate;

    /// A list of directories containing trusted certificates (if authentication is enabled)
    OFList<OFString> m_trustedCertDirs;

    /// A list of files containing trusted certificates (if authentication is enabled)
    OFList<OFString> m_trustedCertFiles;

    /// The file containing the private key (if authentication is enabled)
    OFString m_privateKeyFile;

    /// private key file format
    DcmKeyFileFormat m_privateKeyFileFormat;

    /// File containing the certificate the SCU should use for authentication
    OFString m_certificateFile;

    /// certificate (public key) file format
    DcmKeyFileFormat m_certKeyFileFormat;

    /// password required to open the private key file
    char* m_passwd;

    /// Random seed file used for initializing Pseudo Random Number
    /// Generator (PRNG)
    OFString m_readSeedFile;

    /// Random seed file used for writing updated seed from Pseudo Random Number
    /// Generator back to a file
    OFString m_writeSeedFile;

    /// Denotes how certificates are handled, i.e.\ whether they are required, validated or
    /// only validated if present
    DcmCertificateVerification m_certVerification;

};
#include "tlsstorscu.hpp"

Marco Eichelberg
OFFIS DICOM Team
OFFIS DICOM Team
Posts: 1337
Joined: Tue, 2004-11-02, 17:22
Location: Oldenburg, Germany
Contact:

Re: Design suggestions for extending DcmStorageSCU with TLS support?

#3 Post by Marco Eichelberg » Mon, 2022-05-16, 17:12

I think that the template based mixin just created unneccessary complexity. Actually adding TLS support to a class derived from DcmSCU is quite trivial. There is no example in the public DCMTK yet, but some of the private modules already provide TLS support in DcmSCU based code. Note that this is a command line tool offering the same TLS command line options as, for example, storescu.

Essentially, my DcmSCU based class has one additional member variable and two new methods:

Code: Select all

  /** get reference to handler for secure connections
   *  @return reference to handler for secure connections
   */
  virtual DcmTLSOptions& getTLSOptions()
  {
    return m_tlsOptions;
  }

  /** Active secure connection if compiled with OpenSSL and a secure
   *  connection was requested. Otherwise does nothing.
   *  @param app reference to OFConsoleApplication object that manages the command line options
   *  @param cmd reference to OFCommandLine object that manages the command line options
   *  @return EC_Normal upon success, an error code otherwise
   */
  virtual OFCondition activateTLSifRequested(OFConsoleApplication& app,
                                             OFCommandLine& cmd);

private:

  /// Handler for secure connections
  DcmTLSOptions m_tlsOptions;
The implementation of activateTLSifRequested() is fairly trivial:

Code: Select all

OFCondition MySCU::activateTLSifRequested(OFConsoleApplication& app, OFCommandLine& cmd)
{
    OFCondition result = EC_Normal;
    /* create a secure transport layer if requested and OpenSSL is available */
    if (m_tlsOptions.secureConnectionRequested())
    {
        result = m_tlsOptions.createTransportLayer(NULL, NULL, app, cmd);
        if (result.good())
            useSecureConnection(m_tlsOptions.getTransportLayer());
    }
    return result;
}
All that is now needed are three small additions in main():

Add the command line options:

Code: Select all

  // add TLS specific command line options if (and only if) we are compiling with OpenSSL
  myscu.getTLSOptions().addTLSCommandlineOptions(cmd);
Evaluate the command line options and activate TLS if requested:

Code: Select all

    /* create a secure transport layer if requested and OpenSSL is available */
    status = myscu.activateTLSifRequested(app, cmd);
    if (status.bad())
    {
      ...
    }

     /* negotiate network association with peer */
     status = mppsSCU.negotiateAssociation();
Write back random seed if necessary:

Code: Select all

    myscu.closeAssociation(DCMSCU_RELEASE_ASSOCIATION);
    status = myscu.getTLSOptions().writeRandomSeed();
    if (status.bad())
    {
       ...
    }
Now, if you don't want to write a command line tool, there would be a bit more code involved (similar to the helper class DcmTLSOptions), but DcmSCU can still remain as it is since useSecureConnection() already offers the interface needed.

jogerh
Posts: 20
Joined: Mon, 2022-02-28, 08:55

Re: Design suggestions for extending DcmStorageSCU with TLS support?

#4 Post by jogerh » Mon, 2022-05-16, 17:41

Thank you Marco! This sounds like a better approach. I may have been misled a bit by the design of DcmTLSScu. I'll go back to the drawing board based upon your suggestion. By the way, what is the future of DcmTLSScu? Does it require a stable API, or would it make sense to change it according to your suggestions?

Thanks,
Jøger

Marco Eichelberg
OFFIS DICOM Team
OFFIS DICOM Team
Posts: 1337
Joined: Tue, 2004-11-02, 17:22
Location: Oldenburg, Germany
Contact:

Re: Design suggestions for extending DcmStorageSCU with TLS support?

#5 Post by Marco Eichelberg » Wed, 2022-05-18, 12:00

Other members of the DCMTK team (including the author of DcmTLSSCU) may disagree, but I think that we should declare DcmTLSSCU as deprecated and remove it from the toolkit after one or two stable releases. The problem with DcmTLSSCU is that it can only handle TLS connections, whereas DcmSCU can support both encryted and unencrypted. If I were to spend some effort in improving the code, I would perhaps implement a more comprehensive API for class DcmTLSOptions (perhaps based on the methods currently offered by DcmTLSSCU) so that instances of this class are not limited to parsing the command line options, which is just fine for our command line tools, but not useful for somebody developing a solution based on DCMTK.

jogerh
Posts: 20
Joined: Mon, 2022-02-28, 08:55

Re: Design suggestions for extending DcmStorageSCU with TLS support?

#6 Post by jogerh » Wed, 2022-05-18, 12:15

I agree with you Marco. Implementing useful software with the current DcmTLSSCU is cumbersome because we almost always want to support non-TLS connections as well. DcmTLSSCU also has limited utility as an educational example, and I feel it makes it harder than necessary to learn DCMTK.

Michael Onken
DCMTK Developer
Posts: 1879
Joined: Fri, 2004-11-05, 13:47
Location: Oldenburg, Germany
Contact:

Re: Design suggestions for extending DcmStorageSCU with TLS support?

#7 Post by Michael Onken » Wed, 2022-05-18, 15:57

Hi,

I'm fine with removing the class from the toolkit. It was a first untested approach (more than 10 year ago) that has never been used or even much tested (I guess) :wink:

BR,
Michael

Post Reply

Who is online

Users browsing this forum: Bing [Bot] and 1 guest